RCU and function pointers [LWN.net]

RCU and function pointers

Posted Jun 20, 2025 16:12 UTC (Fri) by error27 (guest, #8346)
In reply to: RCU and function pointers by error27
Parent article: Finding locking bugs with Smatch

I did try this and I ran into a bunch of issues...

It turns out that rcu_read_locks are nestable and the locking code doesn't handle that correctly. I might be able to copy and paste the Smatch preempt code to handle this. It's slightly different because the preempt code warns if we ever have preempt disabled and in this case it's the opposite where we want to want if we're ever missing the lock. You want to lean towards which ever way gives fewer false positives. There are places where recursion could confuse it for example. Writing special modules to track rcu_read_lock() instead or re-using the smatch_locking.c code is a lot of typing but it's doable.

I'm a bit confused by rcu_dereference_rtnl(). If we're holding the rtnl_lock(), does that mean the rcu_read_lock() is held?

to post comments

RCU and function pointers

Posted Jun 20, 2025 20:37 UTC (Fri) by johill (subscriber, #25196) [Link]

as for rcu_dereference_rtnl():

 * rcu_dereference_rtnl - rcu_dereference with debug checking
 * @p: The pointer to read, prior to dereferencing
 *
 * Do an rcu_dereference(p), but check caller either holds rcu_read_lock()
 * or RTNL. Note : Please prefer rtnl_dereference() or rcu_dereference()

You need either, not both. Both would be a bit silly. There are a a lot of calls to rcu_dereference_check() elsewhere which are basically "either RCU or the lock protecting the thing". The reason (IMHO) it says rtnl_dereference() or rcu_dereference() is preferable is that then you have selected either one of the contexts (holding the lock - RTNL in this case - or under RCU protection), but there's always going to be some code that doesn't know the context in whatever call stack it has.

RCU and function pointers

Posted Jun 20, 2025 22:34 UTC (Fri) by neilbrown (subscriber, #359) [Link] (1 responses)

> Writing special modules to track rcu_read_lock() instead or re-using the smatch_locking.c code is a lot of typing but it's doable.

Thanks for digging in to this! Don't try too hard on my account. It was little more than an idle thought because I had seen a d_compare function which uses rcu_dereference but can sometimes be called without rcu_read_locking() - but in practice almost never is (proc_sys_compare) and I wondered how hard it would be for smatch to notice. It seems "not impossible but not trivial" which is a good answer. In this case the anomaly can actually be found simply using sparse as the pointer in question is not marked __rcu.

BTW I tend to think of rcu_read_lock() like a refcount rather than like a read-lock. It is a refcount on "everything". So maybe copying smatch_locking.c wouldn't be the best approach. Does smatch track how many references the code owns to objects? Or does that sound too much like borrow-checking? Could it track references to the RCUniverse? Would that be any easier?

Thanks!

RCU and function pointers

Posted Jun 23, 2025 10:39 UTC (Mon) by error27 (guest, #8346) [Link]

> BTW I tend to think of rcu_read_lock() like a refcount rather than like a read-lock.

This is like preempt. Every spinlock bumps the refcount and people often hold multiple spinlocks at the same time.

With preempt the return states are tracked the same way described here. The one subtlety is that if you take multiple locks it only counts as one refcount bump. It doesn't matter the exact number of refcounts we're holding, only that it's greater than one and that the inc/dec functions pair correctly. So it's simpler to just say +1.

For the caller side, the preempt count is tracked as a number. But again it's not an exact number. If any of the callers are holding one or more spinlocks then that's +1. If we take another spinlock then we pump the count. Say we were holding two locks so we started as +1 and then we drop both locks then we'd go to negative and that's fine too. I guess we might miss a bug but that's not as bad as a false positive.

The difference between preempt count and rcu_read_lock() is that for preempt we only need to find one caller which has preempt disabled, but for rcu_read_lock() we need to find all the callers, otherwise it triggers a false positive. It's a much higher bar.

The way that recursion works in Smatch means we're always going to miss some callers. I guess one idea is that I could periodically delete all the simple recursive calls from the database where a function calls itself? It doesn't solve the halting problem but it probably silences quite a few false positives. But the main thing is that I'd have to create a list of functions which need to be manually silenced. There are about 400 warnings so it's probably a day's work to silence all the warnings.