netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] Questionable RCU/BH usage in cgw_create_job().
@ 2023-10-31 11:23 Sebastian Andrzej Siewior
  2023-10-31 16:14 ` Oliver Hartkopp
  0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-10-31 11:23 UTC (permalink / raw)
  To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can, netdev, Thomas Gleixner

Hi,

I stumbled over this piece in cgw_create_job():
|      /* update modifications with disabled softirq & quit */
|      local_bh_disable();
|      memcpy(&gwj->mod, &mod, sizeof(mod));
|      local_bh_enable();
|      return 0;

unfortunately the comment did not provide much enlightenment for me.
Let's look. That memcpy() overwrites struct cf_mod within struct cgw_job
which is under RCU protection. memcpy() and RCU hardly works as a combo.
But why the local_bh_disable()?

Let's look further. The user of this data structure is can_can_gw_rcv().
There is something like:
|                 /* check for checksum updates */
|                 if (gwj->mod.csumfunc.crc8)
|                         (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);

With optimisation enabled (as in -O2 or so) the compiler will fetch
mod.csumfunc.crc8, do the comparison and if non-NULL use the previously
fetched value and jump there. So one could argue that it is not really
affected by the memcpy() suddenly setting it to NULL. However, adding
any kind of a function in between, say 
|                 /* check for checksum updates */
|                 if (gwj->mod.csumfunc.crc8) {
|+                        trace_event_crc8_sth(cf)
|                         (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
|                 }

will force the compiler to reload mod.csumfunc.crc8. And here is the
possible NULL pointer if overwritten by update in cgw_create_job().

One reload that already happens is the one of mod.modfunc. First at the
top we have:
|         if (gwj->mod.modfunc[0])
|                 nskb = skb_copy(skb, GFP_ATOMIC);
|         else
|                 nskb = skb_clone(skb, GFP_ATOMIC);

Here mod.modfunc[0] is NULL and skb_clone() is invoked. Later if
mod.modfunc has been set to non-NULL value this piece
|         while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
|                 (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);

reloads mod.modfunc and may modify the skb assuming that skb_copy() has
been used earlier.

Looking at this makes me think that the local_bh_disable() has been
added to the memcpy() just to ensure that can_can_gw_rcv() won't run
because it is invoked in a BH disabled context. Clever little trick that
is. But this trick is limited to UP environments…

Am I missing something?

If not, my suggestion would be replacing the bh-off, memcpy part with:
|		old_mod = rcu_replace_pointer(gwj->mod, new_mod, true);
|		kfree_rcu_mightsleep(old_mod);

and doing the needed pointer replacement with for struct cgw_job::mod
and RCU annotation.

Sebastian

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-21 12:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-31 11:23 [RFC] Questionable RCU/BH usage in cgw_create_job() Sebastian Andrzej Siewior
2023-10-31 16:14 ` Oliver Hartkopp
2023-10-31 16:52   ` Sebastian Andrzej Siewior
2023-11-30 16:43     ` Sebastian Andrzej Siewior
2023-11-30 19:56       ` Oliver Hartkopp
2023-12-21 12:43         ` Oliver Hartkopp

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 NNTP newsgroup(s).