On Thu, Jun 18, 2020 at 10:08 PM Dominique Martinet <asmad...@codewreck.org> wrote: > > Alexander Kapshuk wrote on Thu, Jun 18, 2020: > > Address sparse nonderef rcu warnings: > > net/9p/client.c:790:17: warning: incorrect type in argument 1 (different > > address spaces) > > net/9p/client.c:790:17: expected struct spinlock [usertype] *lock > > net/9p/client.c:790:17: got struct spinlock [noderef] <asn:4> * > > net/9p/client.c:792:48: warning: incorrect type in argument 1 (different > > address spaces) > > net/9p/client.c:792:48: expected struct spinlock [usertype] *lock > > net/9p/client.c:792:48: got struct spinlock [noderef] <asn:4> * > > net/9p/client.c:872:17: warning: incorrect type in argument 1 (different > > address spaces) > > net/9p/client.c:872:17: expected struct spinlock [usertype] *lock > > net/9p/client.c:872:17: got struct spinlock [noderef] <asn:4> * > > net/9p/client.c:874:48: warning: incorrect type in argument 1 (different > > address spaces) > > net/9p/client.c:874:48: expected struct spinlock [usertype] *lock > > net/9p/client.c:874:48: got struct spinlock [noderef] <asn:4> * > > > > Signed-off-by: Alexander Kapshuk <alexander.kaps...@gmail.com> > > Thanks for this patch. > From what I can see, there are tons of other parts of the code doing the > same noderef access pattern to access current->sighand->siglock and I > don't see much doing that. > A couple of users justify this by saying SLAB_TYPESAFE_BY_RCU ensures > we'll always get a usable lock which won't be reinitialized however we > access it... It's a bit dubious we'll get the same lock than unlock to > me, so I agree to some change though. > > After a second look I think we should use something like the following: > > if (!lock_task_sighand(current, &flags)) > warn & skip (or some error, we'd null deref if this happened > currently); > recalc_sigpending(); > unlock_task_sighand(current, &flags); > > As you can see, the rcu_read_lock() isn't kept until the unlock so I'm > not sure it will be enough to please sparse, but I've convinced myself > current->sighand cannot change while we hold the lock and there just are > too many such patterns in the kernel. > > Please let me know if I missed something or if there is an ongoing > effort to change how this works; I'll wait for a v2. > > -- > Dominique
Thanks for your prompt response. I too made the same observation of the numerous patterns in the kernel where current->sighand is accessed without being rcu_dereference()'d. For this patch I used kernel/signal.c:1368,1398: __lock_task_sighand() as an example. I will give your suggestion a careful consideration and will get back to you soon. Thanks.
