Hi Sebastian,
thanks for the review!
On 31.10.23 12:23, Sebastian Andrzej Siewior wrote:
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()?
The content of gwj->mod can be overwritten with new modification rules
at runtime. But this update (with memcpy) has to take place when there
is no incoming network traffic.
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.
Replacing a pointer does not copy any data to the cf_mod structure, right?
Best regards,
Oliver
