* [bug report] ASoC: audio-graph-card2: add Codec2Codec support
@ 2024-05-08 15:14 Dan Carpenter
2024-05-09 0:13 ` Kuninori Morimoto
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-05-08 15:14 UTC (permalink / raw
To: kuninori.morimoto.gx; +Cc: linux-sound
Hello Kuninori Morimoto,
Commit c3a15c92a67b ("ASoC: audio-graph-card2: add Codec2Codec
support") from Oct 12, 2021 (linux-next), leads to the following
Smatch static checker warning:
sound/soc/generic/audio-graph-card2.c:1206 graph_count_c2c()
warn: already decremented on line 1206 'lnk->kobj.kref.refcount.refs.counter'
sound/soc/generic/audio-graph-card2.c
1194 static int graph_count_c2c(struct simple_util_priv *priv,
1195 struct device_node *lnk,
1196 struct link_info *li)
1197 {
1198 struct device_node *ports = of_get_parent(lnk);
1199 struct device_node *port0 = lnk;
1200 struct device_node *port1 = of_get_next_child(ports, lnk);
^^^
This calls of_node_put() on lnk.
1201 struct device_node *ep0 = port_to_endpoint(port0);
1202 struct device_node *ep1 = port_to_endpoint(port1);
1203 struct device_node *codec0 = of_graph_get_remote_port(ep0);
1204 struct device_node *codec1 = of_graph_get_remote_port(ep1);
1205
--> 1206 of_node_get(lnk);
So this of_node_get() undoes the put. But if the reference count
dropped to zero then this would be a use afer free.
1207
1208 /*
1209 * codec2codec {
1210 * ports {
1211 * => lnk: port@0 { endpoint { ... }; };
1212 * port@1 { endpoint { ... }; };
1213 * };
1214 * };
1215 */
1216 /*
1217 * DON'T REMOVE platforms
1218 * see
1219 * simple-card.c :: simple_count_noml()
1220 */
1221 li->num[li->link].cpus =
1222 li->num[li->link].platforms = graph_counter(codec0);
1223
1224 li->num[li->link].codecs = graph_counter(codec1);
1225
1226 of_node_put(ports);
1227 of_node_put(port1);
1228 of_node_put(ep0);
1229 of_node_put(ep1);
1230 of_node_put(codec0);
1231 of_node_put(codec1);
1232
1233 return 0;
1234 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: audio-graph-card2: add Codec2Codec support
2024-05-08 15:14 [bug report] ASoC: audio-graph-card2: add Codec2Codec support Dan Carpenter
@ 2024-05-09 0:13 ` Kuninori Morimoto
2024-05-09 6:49 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Kuninori Morimoto @ 2024-05-09 0:13 UTC (permalink / raw
To: Dan Carpenter; +Cc: linux-sound
Hi Dan
> Hello Kuninori Morimoto,
>
> Commit c3a15c92a67b ("ASoC: audio-graph-card2: add Codec2Codec
> support") from Oct 12, 2021 (linux-next), leads to the following
> Smatch static checker warning:
>
> sound/soc/generic/audio-graph-card2.c:1206 graph_count_c2c()
> warn: already decremented on line 1206 'lnk->kobj.kref.refcount.refs.counter'
>
> sound/soc/generic/audio-graph-card2.c
> 1194 static int graph_count_c2c(struct simple_util_priv *priv,
> 1195 struct device_node *lnk,
> 1196 struct link_info *li)
> 1197 {
> 1198 struct device_node *ports = of_get_parent(lnk);
> 1199 struct device_node *port0 = lnk;
> 1200 struct device_node *port1 = of_get_next_child(ports, lnk);
> ^^^
>
> This calls of_node_put() on lnk.
>
> 1201 struct device_node *ep0 = port_to_endpoint(port0);
> 1202 struct device_node *ep1 = port_to_endpoint(port1);
> 1203 struct device_node *codec0 = of_graph_get_remote_port(ep0);
> 1204 struct device_node *codec1 = of_graph_get_remote_port(ep1);
> 1205
> --> 1206 of_node_get(lnk);
>
> So this of_node_get() undoes the put. But if the reference count
> dropped to zero then this would be a use afer free.
Thank you for pointing it.
This "lnk" is used as "port0", and of_node_get(lnk) was for it,
but this function doesn't call of_node_put(port0).
So yes, indeed this of_node_get() is not needed.
Let's remove it.
Thank you for your help !!
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: audio-graph-card2: add Codec2Codec support
2024-05-09 0:13 ` Kuninori Morimoto
@ 2024-05-09 6:49 ` Dan Carpenter
2024-05-09 23:47 ` Kuninori Morimoto
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-05-09 6:49 UTC (permalink / raw
To: Kuninori Morimoto; +Cc: linux-sound
On Thu, May 09, 2024 at 12:13:41AM +0000, Kuninori Morimoto wrote:
>
> Hi Dan
>
> > Hello Kuninori Morimoto,
> >
> > Commit c3a15c92a67b ("ASoC: audio-graph-card2: add Codec2Codec
> > support") from Oct 12, 2021 (linux-next), leads to the following
> > Smatch static checker warning:
> >
> > sound/soc/generic/audio-graph-card2.c:1206 graph_count_c2c()
> > warn: already decremented on line 1206 'lnk->kobj.kref.refcount.refs.counter'
> >
> > sound/soc/generic/audio-graph-card2.c
> > 1194 static int graph_count_c2c(struct simple_util_priv *priv,
> > 1195 struct device_node *lnk,
> > 1196 struct link_info *li)
> > 1197 {
> > 1198 struct device_node *ports = of_get_parent(lnk);
> > 1199 struct device_node *port0 = lnk;
> > 1200 struct device_node *port1 = of_get_next_child(ports, lnk);
> > ^^^
> >
> > This calls of_node_put() on lnk.
> >
> > 1201 struct device_node *ep0 = port_to_endpoint(port0);
> > 1202 struct device_node *ep1 = port_to_endpoint(port1);
> > 1203 struct device_node *codec0 = of_graph_get_remote_port(ep0);
> > 1204 struct device_node *codec1 = of_graph_get_remote_port(ep1);
> > 1205
> > --> 1206 of_node_get(lnk);
> >
> > So this of_node_get() undoes the put. But if the reference count
> > dropped to zero then this would be a use afer free.
>
> Thank you for pointing it.
>
> This "lnk" is used as "port0", and of_node_get(lnk) was for it,
> but this function doesn't call of_node_put(port0).
> So yes, indeed this of_node_get() is not needed.
> Let's remove it.
No no... of_get_next_child() will drop the reference. It is needed.
The point is we should take the reference first before calling
of_get_next_child().
Maybe something like this:
struct device_node *port1 = of_get_next_child(ports, of_node_get(lnk));
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: audio-graph-card2: add Codec2Codec support
2024-05-09 6:49 ` Dan Carpenter
@ 2024-05-09 23:47 ` Kuninori Morimoto
0 siblings, 0 replies; 4+ messages in thread
From: Kuninori Morimoto @ 2024-05-09 23:47 UTC (permalink / raw
To: Dan Carpenter; +Cc: linux-sound
Hi Dan
> > > 1201 struct device_node *ep0 = port_to_endpoint(port0);
> > > 1202 struct device_node *ep1 = port_to_endpoint(port1);
> > > 1203 struct device_node *codec0 = of_graph_get_remote_port(ep0);
> > > 1204 struct device_node *codec1 = of_graph_get_remote_port(ep1);
> > > 1205
> > > --> 1206 of_node_get(lnk);
> > >
> > > So this of_node_get() undoes the put. But if the reference count
> > > dropped to zero then this would be a use afer free.
> >
> > Thank you for pointing it.
> >
> > This "lnk" is used as "port0", and of_node_get(lnk) was for it,
> > but this function doesn't call of_node_put(port0).
> > So yes, indeed this of_node_get() is not needed.
> > Let's remove it.
>
> No no... of_get_next_child() will drop the reference. It is needed.
> The point is we should take the reference first before calling
> of_get_next_child().
>
> Maybe something like this:
>
> struct device_node *port1 = of_get_next_child(ports, of_node_get(lnk));
Oops, oh yes. It need to keep reference.
Thank you for your help, again. I will post fixup patch.
Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-09 23:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-08 15:14 [bug report] ASoC: audio-graph-card2: add Codec2Codec support Dan Carpenter
2024-05-09 0:13 ` Kuninori Morimoto
2024-05-09 6:49 ` Dan Carpenter
2024-05-09 23:47 ` Kuninori Morimoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).