| Message ID | 163752463469.1397.703567874113623042.stgit@bazille.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: Fix misuse of rcu_assign_pointer | expand |
On Sun, Nov 21, 2021 at 02:57:14PM -0500, Chuck Lever wrote: > To address this error: > > CC [M] fs/nfsd/filecache.o > CHECK /home/cel/src/linux/linux/fs/nfsd/filecache.c > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible types in comparison expression (different address spaces): > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net [noderef] __rcu * > /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net * > > The "net" field in struct nfsd_fcache_disposal is not annotated as > requiring an RCU assignment. I am not immediately seeing this field indirected through by RCU readers, though maybe that is happening in some other file. However, it does look like this field is being accessed locklessly by read-side code. What prevents the compiler from applying unfortunate optimizations? See tools/memory-model/Documentation/access-marking.txt in a recent kernel or these LWN articles: https://lwn.net/Articles/816854 and https://lwn.net/Articles/793253. Thanx, Paul > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/filecache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index fdf89fcf1a0c..3fa172f86441 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -772,7 +772,7 @@ nfsd_alloc_fcache_disposal(struct net *net) > static void > nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l) > { > - rcu_assign_pointer(l->net, NULL); > + l->net = NULL; > cancel_work_sync(&l->work); > nfsd_file_dispose_list(&l->freeme); > kfree_rcu(l, rcu); >
> On Nov 21, 2021, at 8:59 PM, Paul E. McKenney <paulmck@kernel.org> wrote: > > On Sun, Nov 21, 2021 at 02:57:14PM -0500, Chuck Lever wrote: >> To address this error: >> >> CC [M] fs/nfsd/filecache.o >> CHECK /home/cel/src/linux/linux/fs/nfsd/filecache.c >> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible types in comparison expression (different address spaces): >> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net [noderef] __rcu * >> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net * >> >> The "net" field in struct nfsd_fcache_disposal is not annotated as >> requiring an RCU assignment. > > I am not immediately seeing this field indirected through by RCU readers, > though maybe that is happening in some other file. > > However, it does look like this field is being accessed locklessly by > read-side code. What prevents the compiler from applying unfortunate > optimizations? > > See tools/memory-model/Documentation/access-marking.txt in a recent > kernel or these LWN articles: https://lwn.net/Articles/816854 and > https://lwn.net/Articles/793253. > > Thanx, Paul Thank you, Paul. I’ll look into it. >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/filecache.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >> index fdf89fcf1a0c..3fa172f86441 100644 >> --- a/fs/nfsd/filecache.c >> +++ b/fs/nfsd/filecache.c >> @@ -772,7 +772,7 @@ nfsd_alloc_fcache_disposal(struct net *net) >> static void >> nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l) >> { >> - rcu_assign_pointer(l->net, NULL); >> + l->net = NULL; >> cancel_work_sync(&l->work); >> nfsd_file_dispose_list(&l->freeme); >> kfree_rcu(l, rcu); >>
> On Nov 21, 2021, at 10:01 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >> >> On Nov 21, 2021, at 8:59 PM, Paul E. McKenney <paulmck@kernel.org> wrote: >> >> On Sun, Nov 21, 2021 at 02:57:14PM -0500, Chuck Lever wrote: >>> To address this error: >>> >>> CC [M] fs/nfsd/filecache.o >>> CHECK /home/cel/src/linux/linux/fs/nfsd/filecache.c >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible types in comparison expression (different address spaces): >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net [noderef] __rcu * >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net * >>> >>> The "net" field in struct nfsd_fcache_disposal is not annotated as >>> requiring an RCU assignment. >> >> I am not immediately seeing this field indirected through by RCU readers, >> though maybe that is happening in some other file. >> >> However, it does look like this field is being accessed locklessly by >> read-side code. What prevents the compiler from applying unfortunate >> optimizations? >> >> See tools/memory-model/Documentation/access-marking.txt in a recent >> kernel or these LWN articles: https://lwn.net/Articles/816854 and >> https://lwn.net/Articles/793253. >> >> Thanx, Paul > > Thank you, Paul. I’ll look into it. > > >>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>> --- >>> fs/nfsd/filecache.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>> index fdf89fcf1a0c..3fa172f86441 100644 >>> --- a/fs/nfsd/filecache.c >>> +++ b/fs/nfsd/filecache.c >>> @@ -772,7 +772,7 @@ nfsd_alloc_fcache_disposal(struct net *net) >>> static void >>> nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l) >>> { >>> - rcu_assign_pointer(l->net, NULL); Hi Trond, 9542e6a643fc ("nfsd: Containerise filecache laundrette") added an rcu_assign_pointer() for a field that is not annotated with __rcu. l->net is assigned and compared only to a non-RCU "struct net *" value. Can you say why you believe rcu_assign_pointer() is necessary here? >>> + l->net = NULL; >>> cancel_work_sync(&l->work); >>> nfsd_file_dispose_list(&l->freeme); >>> kfree_rcu(l, rcu); -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index fdf89fcf1a0c..3fa172f86441 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -772,7 +772,7 @@ nfsd_alloc_fcache_disposal(struct net *net) static void nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l) { - rcu_assign_pointer(l->net, NULL); + l->net = NULL; cancel_work_sync(&l->work); nfsd_file_dispose_list(&l->freeme); kfree_rcu(l, rcu);
To address this error: CC [M] fs/nfsd/filecache.o CHECK /home/cel/src/linux/linux/fs/nfsd/filecache.c /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible types in comparison expression (different address spaces): /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net [noderef] __rcu * /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net * The "net" field in struct nfsd_fcache_disposal is not annotated as requiring an RCU assignment. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/nfsd/filecache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)