RCU and function pointers [LWN.net]

RCU and function pointers

Posted Jun 11, 2025 22:30 UTC (Wed) by neilbrown (subscriber, #359)
Parent article: Finding locking bugs with Smatch

A question for Dan, who I'm sure is following...
Does your current code track RCU read locking as well as the more normal locks? And does it follow flow across function pointers in operations structs?
So could you, for example, query the database to see what inode_operations are called without rcu_read_lock held, and then which implementations of those functions use rcu_dereference as though the lock were held?

to post comments

RCU and function pointers

Posted Jun 12, 2025 7:09 UTC (Thu) by error27 (guest, #8346) [Link] (4 responses)

Hi Neil,

> Does your current code track RCU read locking as well as the more normal locks?

Yes.

> And does it follow flow across function pointers in operations structs?

Yep. When you call a function pointer, then it's recorded as "(struct inode_operations)->atomic_open" in the caller_info table. There is a function_ptr table which lists the functions implement that. The command `smdb.py functions inode_operations atomic_open` gives the list. So I could do:

$ echo "select distinct(ptr) from function_ptr;" | sqlite3 smatch_db.sqlite | grep inode_operations | tee ptrs
$ IFS="
" ; for i in $(cat ptrs) ; do smdb.py $i ; done | grep LOCK

I ran this and it says nothing is holding the rcu_read lock... Huh. When I run "echo "select * from caller_info where type = 9030;" | sqlite3 smatch_db.sqlite | grep rcu_read_lock" it shows that it's being recorded in other places. One thing is that I'm not currently using the caller information to print warnings. It's stored in the database, but I haven't actually used that information yet so I haven't debugged it.

> So could you, for example, query the database to see what inode_operations are called without rcu_read_lock held, and then which implementations of those functions use rcu_dereference as though the lock were held?

Yeah. That's *supposed* to be easy but since I haven't debugged that part I'm probably going to run into surprises. What I do is I start with the simplest, stupidest test and then re-write it from there. So my first draft will be to complain if we call rcu_read_lock_held() and we're not holding the RCU read lock. Then from I'll go through and silence the false positives. There might be places where we check if the read lock is held and don't immediately print an error message, right? It's a stupid check. But the first draft is never going to work anyway. Maybe I'll print a different warning if we call rcu_read_lock_held() and Smatch thinks it can't possibly be held and I wouldn't publish that, but it might be useful for debugging.

Probably the actual check is to only complain if we call rcu_read_lock_held() from a rcu_dereference*() macro.

RCU and function pointers

Posted Jun 20, 2025 16:12 UTC (Fri) by error27 (guest, #8346) [Link] (3 responses)

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?

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.