| Message ID | 20250125-kdevops-v1-1-a76cf79127b8@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | nfsd: validate the nfsd_serv pointer before calling svc_wake_up | expand |
On Sun, 26 Jan 2025, Jeff Layton wrote: > nfsd_file_dispose_list_delayed can be called from the filecache > laundrette, which is shut down after the nfsd threads are shut down and > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > are no threads to wake. > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > svc_serv is not freed until after the filecache laundrette is cancelled. > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > This is only lightly tested, but I think it will fix the bug that > Salvatore reported. > --- > fs/nfsd/filecache.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > struct nfsd_file, nf_gc); > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > + struct svc_serv *serv; > > spin_lock(&l->lock); > list_move_tail(&nf->nf_gc, &l->freeme); > spin_unlock(&l->lock); > - svc_wake_up(nn->nfsd_serv); > + > + /* > + * The filecache laundrette is shut down after the > + * nn->nfsd_serv pointer is cleared, but before the > + * svc_serv is freed. > + */ > + serv = nn->nfsd_serv; I wonder if this should be READ_ONCE() to tell the compiler that we could race with clearing nn->nfsd_serv. Would the comment still be needed? Otherwise: Reviewed-by: NeilBrown <neilb@suse.de> Thanks, NeilBrown > + if (serv) > + svc_wake_up(serv); > } > } > > > --- > base-commit: 7541a5b8073cf0d9e2d288cac581f1aa6c11671d > change-id: 20250125-kdevops-0989825ae8db > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> > >
On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > On Sun, 26 Jan 2025, Jeff Layton wrote: > > nfsd_file_dispose_list_delayed can be called from the filecache > > laundrette, which is shut down after the nfsd threads are shut down and > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > are no threads to wake. > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > This is only lightly tested, but I think it will fix the bug that > > Salvatore reported. > > --- > > fs/nfsd/filecache.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > struct nfsd_file, nf_gc); > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > + struct svc_serv *serv; > > > > spin_lock(&l->lock); > > list_move_tail(&nf->nf_gc, &l->freeme); > > spin_unlock(&l->lock); > > - svc_wake_up(nn->nfsd_serv); > > + > > + /* > > + * The filecache laundrette is shut down after the > > + * nn->nfsd_serv pointer is cleared, but before the > > + * svc_serv is freed. > > + */ > > + serv = nn->nfsd_serv; > > I wonder if this should be READ_ONCE() to tell the compiler that we > could race with clearing nn->nfsd_serv. Would the comment still be > needed? > I think we need a comment at least. The linkage between the laundrette and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() doesn't convey that well, and is unnecessary here. > Otherwise: > > Reviewed-by: NeilBrown <neilb@suse.de> > Thanks! > > > + if (serv) > > + svc_wake_up(serv); > > } > > } > > > > > > --- > > base-commit: 7541a5b8073cf0d9e2d288cac581f1aa6c11671d > > change-id: 20250125-kdevops-0989825ae8db > > > > Best regards, > > -- > > Jeff Layton <jlayton@kernel.org> > > > > >
From: Chuck Lever <chuck.lever@oracle.com> On Sat, 25 Jan 2025 20:13:18 -0500, Jeff Layton wrote: > nfsd_file_dispose_list_delayed can be called from the filecache > laundrette, which is shut down after the nfsd threads are shut down and > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > are no threads to wake. > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > svc_serv is not freed until after the filecache laundrette is cancelled. > > [...] Applied to nfsd-testing, thanks! [1/1] nfsd: validate the nfsd_serv pointer before calling svc_wake_up commit: a71d376104bd893a3fc349c5ffeb23a66ec78bcc -- Chuck Lever
On Sun, 26 Jan 2025, Jeff Layton wrote: > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > laundrette, which is shut down after the nfsd threads are shut down and > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > > are no threads to wake. > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > This is only lightly tested, but I think it will fix the bug that > > > Salvatore reported. > > > --- > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > --- a/fs/nfsd/filecache.c > > > +++ b/fs/nfsd/filecache.c > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > > struct nfsd_file, nf_gc); > > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > + struct svc_serv *serv; > > > > > > spin_lock(&l->lock); > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > spin_unlock(&l->lock); > > > - svc_wake_up(nn->nfsd_serv); > > > + > > > + /* > > > + * The filecache laundrette is shut down after the > > > + * nn->nfsd_serv pointer is cleared, but before the > > > + * svc_serv is freed. > > > + */ > > > + serv = nn->nfsd_serv; > > > > I wonder if this should be READ_ONCE() to tell the compiler that we > > could race with clearing nn->nfsd_serv. Would the comment still be > > needed? > > > > I think we need a comment at least. The linkage between the laundrette > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > doesn't convey that well, and is unnecessary here. Why do you say "is unnecessary here" ? If the code were if (nn->nfsd_serv) svc_wake_up(nn->nfsd_serv); that would be wrong as nn->nfds_serv could be set to NULL between the two. And the C compile is allowed to load the value twice because the C memory model declares that would have the same effect. While I doubt it would actually change how the code is compiled, I think we should have READ_ONCE() here (and I've been wrong before about what the compiler will actually do). Thanks, NeilBrown
On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > On Sun, 26 Jan 2025, Jeff Layton wrote: > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > laundrette, which is shut down after the nfsd threads are shut down and > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > > > are no threads to wake. > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > This is only lightly tested, but I think it will fix the bug that > > > > Salvatore reported. > > > > --- > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > --- a/fs/nfsd/filecache.c > > > > +++ b/fs/nfsd/filecache.c > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > > > struct nfsd_file, nf_gc); > > > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > + struct svc_serv *serv; > > > > > > > > spin_lock(&l->lock); > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > spin_unlock(&l->lock); > > > > - svc_wake_up(nn->nfsd_serv); > > > > + > > > > + /* > > > > + * The filecache laundrette is shut down after the > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > + * svc_serv is freed. > > > > + */ > > > > + serv = nn->nfsd_serv; > > > > > > I wonder if this should be READ_ONCE() to tell the compiler that we > > > could race with clearing nn->nfsd_serv. Would the comment still be > > > needed? > > > > > > > I think we need a comment at least. The linkage between the laundrette > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > doesn't convey that well, and is unnecessary here. > > Why do you say "is unnecessary here" ? > If the code were > if (nn->nfsd_serv) > svc_wake_up(nn->nfsd_serv); > that would be wrong as nn->nfds_serv could be set to NULL between the > two. > And the C compile is allowed to load the value twice because the C memory > model declares that would have the same effect. > While I doubt it would actually change how the code is compiled, I think > we should have READ_ONCE() here (and I've been wrong before about what > the compiler will actually do). > > It's unnecessary because the outcome of either case is acceptable. When racing with shutdown, either it's NULL and the laundrette won't call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, the call to svc_wake_up() will be a no-op because the threads are shut down. The vastly common case in this code is that this pointer will be non- NULL, because the server is running (i.e. not racing with shutdown). I don't see the need in making all of those accesses volatile.
On Mon, 27 Jan 2025, Jeff Layton wrote: > On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > > laundrette, which is shut down after the nfsd threads are shut down and > > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > > > > are no threads to wake. > > > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > > > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > --- > > > > > This is only lightly tested, but I think it will fix the bug that > > > > > Salvatore reported. > > > > > --- > > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > > --- a/fs/nfsd/filecache.c > > > > > +++ b/fs/nfsd/filecache.c > > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > > > > struct nfsd_file, nf_gc); > > > > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > > + struct svc_serv *serv; > > > > > > > > > > spin_lock(&l->lock); > > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > > spin_unlock(&l->lock); > > > > > - svc_wake_up(nn->nfsd_serv); > > > > > + > > > > > + /* > > > > > + * The filecache laundrette is shut down after the > > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > > + * svc_serv is freed. > > > > > + */ > > > > > + serv = nn->nfsd_serv; > > > > > > > > I wonder if this should be READ_ONCE() to tell the compiler that we > > > > could race with clearing nn->nfsd_serv. Would the comment still be > > > > needed? > > > > > > > > > > I think we need a comment at least. The linkage between the laundrette > > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > > doesn't convey that well, and is unnecessary here. > > > > Why do you say "is unnecessary here" ? > > If the code were > > if (nn->nfsd_serv) > > svc_wake_up(nn->nfsd_serv); > > that would be wrong as nn->nfds_serv could be set to NULL between the > > two. > > And the C compile is allowed to load the value twice because the C memory > > model declares that would have the same effect. > > While I doubt it would actually change how the code is compiled, I think > > we should have READ_ONCE() here (and I've been wrong before about what > > the compiler will actually do). > > > > > > It's unnecessary because the outcome of either case is acceptable. > > When racing with shutdown, either it's NULL and the laundrette won't > call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, > the call to svc_wake_up() will be a no-op because the threads are shut > down. > > The vastly common case in this code is that this pointer will be non- > NULL, because the server is running (i.e. not racing with shutdown). I > don't see the need in making all of those accesses volatile. One of us is confused. I hope it isn't me. The hypothetical problem I see is that the C compiler could generate code to load the value "nn->nfsd_serv" twice. The first time it is not NULL, the second time it is NULL. The first is used for the test, the second is passed to svc_wake_up(). Unlikely though this is, it is possible and READ_ONCE() is designed precisely to prevent this. To quote from include/asm-generic/rwonce.h it will "Prevent the compiler from merging or refetching reads" A "volatile" access does not add any cost (in this case). What it does is break any aliasing that the compile might have deduced. Even if the compiler thinks it has "nn->nfsd_serv" in a register, it won't think it has the result of READ_ONCE(nn->nfsd_serv) in that register. And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it won't decide that it can just read nn->nfsd_serv again. It MUST keep the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not needed any more. NeilBrown
On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: > On Mon, 27 Jan 2025, Jeff Layton wrote: > > On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > > > laundrette, which is shut down after the nfsd threads are shut down and > > > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > > > > > are no threads to wake. > > > > > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > > > > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > --- > > > > > > This is only lightly tested, but I think it will fix the bug that > > > > > > Salvatore reported. > > > > > > --- > > > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > > > --- a/fs/nfsd/filecache.c > > > > > > +++ b/fs/nfsd/filecache.c > > > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > > > > > struct nfsd_file, nf_gc); > > > > > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > > > + struct svc_serv *serv; > > > > > > > > > > > > spin_lock(&l->lock); > > > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > > > spin_unlock(&l->lock); > > > > > > - svc_wake_up(nn->nfsd_serv); > > > > > > + > > > > > > + /* > > > > > > + * The filecache laundrette is shut down after the > > > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > > > + * svc_serv is freed. > > > > > > + */ > > > > > > + serv = nn->nfsd_serv; > > > > > > > > > > I wonder if this should be READ_ONCE() to tell the compiler that we > > > > > could race with clearing nn->nfsd_serv. Would the comment still be > > > > > needed? > > > > > > > > > > > > > I think we need a comment at least. The linkage between the laundrette > > > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > > > doesn't convey that well, and is unnecessary here. > > > > > > Why do you say "is unnecessary here" ? > > > If the code were > > > if (nn->nfsd_serv) > > > svc_wake_up(nn->nfsd_serv); > > > that would be wrong as nn->nfds_serv could be set to NULL between the > > > two. > > > And the C compile is allowed to load the value twice because the C memory > > > model declares that would have the same effect. > > > While I doubt it would actually change how the code is compiled, I think > > > we should have READ_ONCE() here (and I've been wrong before about what > > > the compiler will actually do). > > > > > > > > > > It's unnecessary because the outcome of either case is acceptable. > > > > When racing with shutdown, either it's NULL and the laundrette won't > > call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, > > the call to svc_wake_up() will be a no-op because the threads are shut > > down. > > > > The vastly common case in this code is that this pointer will be non- > > NULL, because the server is running (i.e. not racing with shutdown). I > > don't see the need in making all of those accesses volatile. > > One of us is confused. I hope it isn't me. > It's probably me. I think you have a much better understanding of compiler design than I do. Still... > The hypothetical problem I see is that the C compiler could generate > code to load the value "nn->nfsd_serv" twice. The first time it is not > NULL, the second time it is NULL. > The first is used for the test, the second is passed to svc_wake_up(). > > Unlikely though this is, it is possible and READ_ONCE() is designed > precisely to prevent this. > To quote from include/asm-generic/rwonce.h it will > "Prevent the compiler from merging or refetching reads" > > A "volatile" access does not add any cost (in this case). What it does > is break any aliasing that the compile might have deduced. > Even if the compiler thinks it has "nn->nfsd_serv" in a register, it > won't think it has the result of READ_ONCE(nn->nfsd_serv) in that register. > And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it > won't decide that it can just read nn->nfsd_serv again. It MUST keep > the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not needed > any more. I'm mainly just considering the resulting pointer. There are two possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid pointer that points to the svc_serv, or it's NULL. The resulting code can handle either case, so it doesn't seem like adding READ_ONCE() will create any material difference here. Maybe I should ask it this way: What bad outcome could result if we don't add READ_ONCE() here?
On 1/27/25 8:07 AM, Jeff Layton wrote: > On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: >> On Mon, 27 Jan 2025, Jeff Layton wrote: >>> On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: >>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>> On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: >>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>>>> nfsd_file_dispose_list_delayed can be called from the filecache >>>>>>> laundrette, which is shut down after the nfsd threads are shut down and >>>>>>> the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there >>>>>>> are no threads to wake. >>>>>>> >>>>>>> Ensure that the nn->nfsd_serv pointer is non-NULL before calling >>>>>>> svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the >>>>>>> svc_serv is not freed until after the filecache laundrette is cancelled. >>>>>>> >>>>>>> Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") >>>>>>> Reported-by: Salvatore Bonaccorso <carnil@debian.org> >>>>>>> Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ >>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>>>>> --- >>>>>>> This is only lightly tested, but I think it will fix the bug that >>>>>>> Salvatore reported. >>>>>>> --- >>>>>>> fs/nfsd/filecache.c | 11 ++++++++++- >>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>>>>>> index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 >>>>>>> --- a/fs/nfsd/filecache.c >>>>>>> +++ b/fs/nfsd/filecache.c >>>>>>> @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) >>>>>>> struct nfsd_file, nf_gc); >>>>>>> struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); >>>>>>> struct nfsd_fcache_disposal *l = nn->fcache_disposal; >>>>>>> + struct svc_serv *serv; >>>>>>> >>>>>>> spin_lock(&l->lock); >>>>>>> list_move_tail(&nf->nf_gc, &l->freeme); >>>>>>> spin_unlock(&l->lock); >>>>>>> - svc_wake_up(nn->nfsd_serv); >>>>>>> + >>>>>>> + /* >>>>>>> + * The filecache laundrette is shut down after the >>>>>>> + * nn->nfsd_serv pointer is cleared, but before the >>>>>>> + * svc_serv is freed. >>>>>>> + */ >>>>>>> + serv = nn->nfsd_serv; >>>>>> >>>>>> I wonder if this should be READ_ONCE() to tell the compiler that we >>>>>> could race with clearing nn->nfsd_serv. Would the comment still be >>>>>> needed? >>>>>> >>>>> >>>>> I think we need a comment at least. The linkage between the laundrette >>>>> and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() >>>>> doesn't convey that well, and is unnecessary here. >>>> >>>> Why do you say "is unnecessary here" ? >>>> If the code were >>>> if (nn->nfsd_serv) >>>> svc_wake_up(nn->nfsd_serv); >>>> that would be wrong as nn->nfds_serv could be set to NULL between the >>>> two. >>>> And the C compile is allowed to load the value twice because the C memory >>>> model declares that would have the same effect. >>>> While I doubt it would actually change how the code is compiled, I think >>>> we should have READ_ONCE() here (and I've been wrong before about what >>>> the compiler will actually do). >>>> >>>> >>> >>> It's unnecessary because the outcome of either case is acceptable. >>> >>> When racing with shutdown, either it's NULL and the laundrette won't >>> call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, >>> the call to svc_wake_up() will be a no-op because the threads are shut >>> down. >>> >>> The vastly common case in this code is that this pointer will be non- >>> NULL, because the server is running (i.e. not racing with shutdown). I >>> don't see the need in making all of those accesses volatile. >> >> One of us is confused. I hope it isn't me. >> > > It's probably me. I think you have a much better understanding of > compiler design than I do. Still... > >> The hypothetical problem I see is that the C compiler could generate >> code to load the value "nn->nfsd_serv" twice. The first time it is not >> NULL, the second time it is NULL. >> The first is used for the test, the second is passed to svc_wake_up(). >> >> Unlikely though this is, it is possible and READ_ONCE() is designed >> precisely to prevent this. >> To quote from include/asm-generic/rwonce.h it will >> "Prevent the compiler from merging or refetching reads" >> >> A "volatile" access does not add any cost (in this case). What it does >> is break any aliasing that the compile might have deduced. >> Even if the compiler thinks it has "nn->nfsd_serv" in a register, it >> won't think it has the result of READ_ONCE(nn->nfsd_serv) in that register. >> And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it >> won't decide that it can just read nn->nfsd_serv again. It MUST keep >> the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not needed >> any more. > > I'm mainly just considering the resulting pointer. There are two > possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid > pointer that points to the svc_serv, or it's NULL. The resulting code > can handle either case, so it doesn't seem like adding READ_ONCE() will > create any material difference here. > > Maybe I should ask it this way: What bad outcome could result if we > don't add READ_ONCE() here? Neil just described it. The compiler would generate two load operations, one for the test and one for the function call argument. The first load can retrieve a non-NULL address, and the second a NULL address. I agree a READ_ONCE() is necessary.
On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: > On 1/27/25 8:07 AM, Jeff Layton wrote: > > On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: > > > On Mon, 27 Jan 2025, Jeff Layton wrote: > > > > On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > > > > > laundrette, which is shut down after the nfsd threads are shut down and > > > > > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > > > > > > > > are no threads to wake. > > > > > > > > > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > > > > > > > > svc_serv is not freed until after the filecache laundrette is cancelled. > > > > > > > > > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") > > > > > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > > > > > Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > > > --- > > > > > > > > This is only lightly tested, but I think it will fix the bug that > > > > > > > > Salvatore reported. > > > > > > > > --- > > > > > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > > > > > index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > > > > > --- a/fs/nfsd/filecache.c > > > > > > > > +++ b/fs/nfsd/filecache.c > > > > > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) > > > > > > > > struct nfsd_file, nf_gc); > > > > > > > > struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > > > > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > > > > > + struct svc_serv *serv; > > > > > > > > > > > > > > > > spin_lock(&l->lock); > > > > > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > > > > > spin_unlock(&l->lock); > > > > > > > > - svc_wake_up(nn->nfsd_serv); > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * The filecache laundrette is shut down after the > > > > > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > > > > > + * svc_serv is freed. > > > > > > > > + */ > > > > > > > > + serv = nn->nfsd_serv; > > > > > > > > > > > > > > I wonder if this should be READ_ONCE() to tell the compiler that we > > > > > > > could race with clearing nn->nfsd_serv. Would the comment still be > > > > > > > needed? > > > > > > > > > > > > > > > > > > > I think we need a comment at least. The linkage between the laundrette > > > > > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > > > > > doesn't convey that well, and is unnecessary here. > > > > > > > > > > Why do you say "is unnecessary here" ? > > > > > If the code were > > > > > if (nn->nfsd_serv) > > > > > svc_wake_up(nn->nfsd_serv); > > > > > that would be wrong as nn->nfds_serv could be set to NULL between the > > > > > two. > > > > > And the C compile is allowed to load the value twice because the C memory > > > > > model declares that would have the same effect. > > > > > While I doubt it would actually change how the code is compiled, I think > > > > > we should have READ_ONCE() here (and I've been wrong before about what > > > > > the compiler will actually do). > > > > > > > > > > > > > > > > > > It's unnecessary because the outcome of either case is acceptable. > > > > > > > > When racing with shutdown, either it's NULL and the laundrette won't > > > > call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, > > > > the call to svc_wake_up() will be a no-op because the threads are shut > > > > down. > > > > > > > > The vastly common case in this code is that this pointer will be non- > > > > NULL, because the server is running (i.e. not racing with shutdown). I > > > > don't see the need in making all of those accesses volatile. > > > > > > One of us is confused. I hope it isn't me. > > > > > > > It's probably me. I think you have a much better understanding of > > compiler design than I do. Still... > > > > > The hypothetical problem I see is that the C compiler could generate > > > code to load the value "nn->nfsd_serv" twice. The first time it is not > > > NULL, the second time it is NULL. > > > The first is used for the test, the second is passed to svc_wake_up(). > > > > > > Unlikely though this is, it is possible and READ_ONCE() is designed > > > precisely to prevent this. > > > To quote from include/asm-generic/rwonce.h it will > > > "Prevent the compiler from merging or refetching reads" > > > > > > A "volatile" access does not add any cost (in this case). What it does > > > is break any aliasing that the compile might have deduced. > > > Even if the compiler thinks it has "nn->nfsd_serv" in a register, it > > > won't think it has the result of READ_ONCE(nn->nfsd_serv) in that register. > > > And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it > > > won't decide that it can just read nn->nfsd_serv again. It MUST keep > > > the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not needed > > > any more. > > > > I'm mainly just considering the resulting pointer. There are two > > possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid > > pointer that points to the svc_serv, or it's NULL. The resulting code > > can handle either case, so it doesn't seem like adding READ_ONCE() will > > create any material difference here. > > > > Maybe I should ask it this way: What bad outcome could result if we > > don't add READ_ONCE() here? > > Neil just described it. The compiler would generate two load operations, > one for the test and one for the function call argument. The first load > can retrieve a non-NULL address, and the second a NULL address. > > I agree a READ_ONCE() is necessary. > > Now I'm confused: struct svc_serv *serv; [...] /* * The filecache laundrette is shut down after the * nn->nfsd_serv pointer is cleared, but before the * svc_serv is freed. */ serv = nn->nfsd_serv; if (serv) svc_wake_up(serv); This code is explicitly asking to fetch nn->nfsd_serv into the serv variable, and then is testing that copy of the pointer and passing it into svc_wake_up(). How is the compiler allowed to suddenly refetch a NULL pointer into serv after testing that serv is non-NULL?
On 1/27/25 8:32 AM, Jeff Layton wrote: > On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: >> On 1/27/25 8:07 AM, Jeff Layton wrote: >>> On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: >>>> On Mon, 27 Jan 2025, Jeff Layton wrote: >>>>> On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: >>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>>>> On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: >>>>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>>>>>> nfsd_file_dispose_list_delayed can be called from the filecache >>>>>>>>> laundrette, which is shut down after the nfsd threads are shut down and >>>>>>>>> the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there >>>>>>>>> are no threads to wake. >>>>>>>>> >>>>>>>>> Ensure that the nn->nfsd_serv pointer is non-NULL before calling >>>>>>>>> svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the >>>>>>>>> svc_serv is not freed until after the filecache laundrette is cancelled. >>>>>>>>> >>>>>>>>> Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") >>>>>>>>> Reported-by: Salvatore Bonaccorso <carnil@debian.org> >>>>>>>>> Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ >>>>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>>>>>>> --- >>>>>>>>> This is only lightly tested, but I think it will fix the bug that >>>>>>>>> Salvatore reported. >>>>>>>>> --- >>>>>>>>> fs/nfsd/filecache.c | 11 ++++++++++- >>>>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>>>>>>>> index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 >>>>>>>>> --- a/fs/nfsd/filecache.c >>>>>>>>> +++ b/fs/nfsd/filecache.c >>>>>>>>> @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) >>>>>>>>> struct nfsd_file, nf_gc); >>>>>>>>> struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); >>>>>>>>> struct nfsd_fcache_disposal *l = nn->fcache_disposal; >>>>>>>>> + struct svc_serv *serv; >>>>>>>>> >>>>>>>>> spin_lock(&l->lock); >>>>>>>>> list_move_tail(&nf->nf_gc, &l->freeme); >>>>>>>>> spin_unlock(&l->lock); >>>>>>>>> - svc_wake_up(nn->nfsd_serv); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * The filecache laundrette is shut down after the >>>>>>>>> + * nn->nfsd_serv pointer is cleared, but before the >>>>>>>>> + * svc_serv is freed. >>>>>>>>> + */ >>>>>>>>> + serv = nn->nfsd_serv; >>>>>>>> >>>>>>>> I wonder if this should be READ_ONCE() to tell the compiler that we >>>>>>>> could race with clearing nn->nfsd_serv. Would the comment still be >>>>>>>> needed? >>>>>>>> >>>>>>> >>>>>>> I think we need a comment at least. The linkage between the laundrette >>>>>>> and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() >>>>>>> doesn't convey that well, and is unnecessary here. >>>>>> >>>>>> Why do you say "is unnecessary here" ? >>>>>> If the code were >>>>>> if (nn->nfsd_serv) >>>>>> svc_wake_up(nn->nfsd_serv); >>>>>> that would be wrong as nn->nfds_serv could be set to NULL between the >>>>>> two. >>>>>> And the C compile is allowed to load the value twice because the C memory >>>>>> model declares that would have the same effect. >>>>>> While I doubt it would actually change how the code is compiled, I think >>>>>> we should have READ_ONCE() here (and I've been wrong before about what >>>>>> the compiler will actually do). >>>>>> >>>>>> >>>>> >>>>> It's unnecessary because the outcome of either case is acceptable. >>>>> >>>>> When racing with shutdown, either it's NULL and the laundrette won't >>>>> call svc_wake_up(), or it's non-NULL and it will. In the non-NULL case, >>>>> the call to svc_wake_up() will be a no-op because the threads are shut >>>>> down. >>>>> >>>>> The vastly common case in this code is that this pointer will be non- >>>>> NULL, because the server is running (i.e. not racing with shutdown). I >>>>> don't see the need in making all of those accesses volatile. >>>> >>>> One of us is confused. I hope it isn't me. >>>> >>> >>> It's probably me. I think you have a much better understanding of >>> compiler design than I do. Still... >>> >>>> The hypothetical problem I see is that the C compiler could generate >>>> code to load the value "nn->nfsd_serv" twice. The first time it is not >>>> NULL, the second time it is NULL. >>>> The first is used for the test, the second is passed to svc_wake_up(). >>>> >>>> Unlikely though this is, it is possible and READ_ONCE() is designed >>>> precisely to prevent this. >>>> To quote from include/asm-generic/rwonce.h it will >>>> "Prevent the compiler from merging or refetching reads" >>>> >>>> A "volatile" access does not add any cost (in this case). What it does >>>> is break any aliasing that the compile might have deduced. >>>> Even if the compiler thinks it has "nn->nfsd_serv" in a register, it >>>> won't think it has the result of READ_ONCE(nn->nfsd_serv) in that register. >>>> And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it >>>> won't decide that it can just read nn->nfsd_serv again. It MUST keep >>>> the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not needed >>>> any more. >>> >>> I'm mainly just considering the resulting pointer. There are two >>> possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid >>> pointer that points to the svc_serv, or it's NULL. The resulting code >>> can handle either case, so it doesn't seem like adding READ_ONCE() will >>> create any material difference here. >>> >>> Maybe I should ask it this way: What bad outcome could result if we >>> don't add READ_ONCE() here? >> >> Neil just described it. The compiler would generate two load operations, >> one for the test and one for the function call argument. The first load >> can retrieve a non-NULL address, and the second a NULL address. >> >> I agree a READ_ONCE() is necessary. >> >> > > Now I'm confused: > > struct svc_serv *serv; > > [...] > > /* > * The filecache laundrette is shut down after the > * nn->nfsd_serv pointer is cleared, but before the > * svc_serv is freed. > */ > serv = nn->nfsd_serv; > if (serv) > svc_wake_up(serv); > > This code is explicitly asking to fetch nn->nfsd_serv into the serv > variable, and then is testing that copy of the pointer and passing it > into svc_wake_up(). > > How is the compiler allowed to suddenly refetch a NULL pointer into > serv after testing that serv is non-NULL? There's nothing here that tells the compiler it is not allowed to optimize this into two separate fetches if it feels that is better code. READ_ONCE is what tells the compiler we do not want that re- organization /ever/.
On 1/27/25 8:39 AM, Chuck Lever wrote: > On 1/27/25 8:32 AM, Jeff Layton wrote: >> On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: >>> On 1/27/25 8:07 AM, Jeff Layton wrote: >>>> On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: >>>>> On Mon, 27 Jan 2025, Jeff Layton wrote: >>>>>> On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: >>>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>>>>> On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: >>>>>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: >>>>>>>>>> nfsd_file_dispose_list_delayed can be called from the filecache >>>>>>>>>> laundrette, which is shut down after the nfsd threads are shut >>>>>>>>>> down and >>>>>>>>>> the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL >>>>>>>>>> then there >>>>>>>>>> are no threads to wake. >>>>>>>>>> >>>>>>>>>> Ensure that the nn->nfsd_serv pointer is non-NULL before calling >>>>>>>>>> svc_wake_up in nfsd_file_dispose_list_delayed. This is safe >>>>>>>>>> since the >>>>>>>>>> svc_serv is not freed until after the filecache laundrette is >>>>>>>>>> cancelled. >>>>>>>>>> >>>>>>>>>> Fixes: ffb402596147 ("nfsd: Don't leave work of closing files >>>>>>>>>> to a work queue") >>>>>>>>>> Reported-by: Salvatore Bonaccorso <carnil@debian.org> >>>>>>>>>> Closes: https://lore.kernel.org/linux- >>>>>>>>>> nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ >>>>>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>>>>>>>> --- >>>>>>>>>> This is only lightly tested, but I think it will fix the bug that >>>>>>>>>> Salvatore reported. >>>>>>>>>> --- >>>>>>>>>> fs/nfsd/filecache.c | 11 ++++++++++- >>>>>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>>>>>>>>> index >>>>>>>>>> e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 >>>>>>>>>> --- a/fs/nfsd/filecache.c >>>>>>>>>> +++ b/fs/nfsd/filecache.c >>>>>>>>>> @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct >>>>>>>>>> list_head *dispose) >>>>>>>>>> struct nfsd_file, nf_gc); >>>>>>>>>> struct nfsd_net *nn = net_generic(nf->nf_net, >>>>>>>>>> nfsd_net_id); >>>>>>>>>> struct nfsd_fcache_disposal *l = nn->fcache_disposal; >>>>>>>>>> + struct svc_serv *serv; >>>>>>>>>> spin_lock(&l->lock); >>>>>>>>>> list_move_tail(&nf->nf_gc, &l->freeme); >>>>>>>>>> spin_unlock(&l->lock); >>>>>>>>>> - svc_wake_up(nn->nfsd_serv); >>>>>>>>>> + >>>>>>>>>> + /* >>>>>>>>>> + * The filecache laundrette is shut down after the >>>>>>>>>> + * nn->nfsd_serv pointer is cleared, but before the >>>>>>>>>> + * svc_serv is freed. >>>>>>>>>> + */ >>>>>>>>>> + serv = nn->nfsd_serv; >>>>>>>>> >>>>>>>>> I wonder if this should be READ_ONCE() to tell the compiler >>>>>>>>> that we >>>>>>>>> could race with clearing nn->nfsd_serv. Would the comment >>>>>>>>> still be >>>>>>>>> needed? >>>>>>>>> >>>>>>>> >>>>>>>> I think we need a comment at least. The linkage between the >>>>>>>> laundrette >>>>>>>> and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() >>>>>>>> doesn't convey that well, and is unnecessary here. >>>>>>> >>>>>>> Why do you say "is unnecessary here" ? >>>>>>> If the code were >>>>>>> if (nn->nfsd_serv) >>>>>>> svc_wake_up(nn->nfsd_serv); >>>>>>> that would be wrong as nn->nfds_serv could be set to NULL between >>>>>>> the >>>>>>> two. >>>>>>> And the C compile is allowed to load the value twice because the >>>>>>> C memory >>>>>>> model declares that would have the same effect. >>>>>>> While I doubt it would actually change how the code is compiled, >>>>>>> I think >>>>>>> we should have READ_ONCE() here (and I've been wrong before about >>>>>>> what >>>>>>> the compiler will actually do). >>>>>>> >>>>>>> >>>>>> >>>>>> It's unnecessary because the outcome of either case is acceptable. >>>>>> >>>>>> When racing with shutdown, either it's NULL and the laundrette won't >>>>>> call svc_wake_up(), or it's non-NULL and it will. In the non-NULL >>>>>> case, >>>>>> the call to svc_wake_up() will be a no-op because the threads are >>>>>> shut >>>>>> down. >>>>>> >>>>>> The vastly common case in this code is that this pointer will be non- >>>>>> NULL, because the server is running (i.e. not racing with >>>>>> shutdown). I >>>>>> don't see the need in making all of those accesses volatile. >>>>> >>>>> One of us is confused. I hope it isn't me. >>>>> >>>> >>>> It's probably me. I think you have a much better understanding of >>>> compiler design than I do. Still... >>>> >>>>> The hypothetical problem I see is that the C compiler could generate >>>>> code to load the value "nn->nfsd_serv" twice. The first time it is >>>>> not >>>>> NULL, the second time it is NULL. >>>>> The first is used for the test, the second is passed to svc_wake_up(). >>>>> >>>>> Unlikely though this is, it is possible and READ_ONCE() is designed >>>>> precisely to prevent this. >>>>> To quote from include/asm-generic/rwonce.h it will >>>>> "Prevent the compiler from merging or refetching reads" >>>>> >>>>> A "volatile" access does not add any cost (in this case). What it >>>>> does >>>>> is break any aliasing that the compile might have deduced. >>>>> Even if the compiler thinks it has "nn->nfsd_serv" in a register, it >>>>> won't think it has the result of READ_ONCE(nn->nfsd_serv) in that >>>>> register. >>>>> And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it >>>>> won't decide that it can just read nn->nfsd_serv again. It MUST keep >>>>> the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not >>>>> needed >>>>> any more. >>>> >>>> I'm mainly just considering the resulting pointer. There are two >>>> possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid >>>> pointer that points to the svc_serv, or it's NULL. The resulting code >>>> can handle either case, so it doesn't seem like adding READ_ONCE() will >>>> create any material difference here. >>>> >>>> Maybe I should ask it this way: What bad outcome could result if we >>>> don't add READ_ONCE() here? >>> >>> Neil just described it. The compiler would generate two load operations, >>> one for the test and one for the function call argument. The first load >>> can retrieve a non-NULL address, and the second a NULL address. >>> >>> I agree a READ_ONCE() is necessary. >>> >>> >> >> Now I'm confused: >> >> struct svc_serv *serv; >> >> [...] >> >> /* >> * The filecache laundrette is shut down after the >> * nn->nfsd_serv pointer is cleared, but before the >> * svc_serv is freed. >> */ >> serv = nn->nfsd_serv; >> if (serv) >> svc_wake_up(serv); >> >> This code is explicitly asking to fetch nn->nfsd_serv into the serv >> variable, and then is testing that copy of the pointer and passing it >> into svc_wake_up(). >> >> How is the compiler allowed to suddenly refetch a NULL pointer into >> serv after testing that serv is non-NULL? > > There's nothing here that tells the compiler it is not allowed to > optimize this into two separate fetches if it feels that is better > code. READ_ONCE is what tells the compiler we do not want that re- > organization /ever/. > > Well, I think you can argue that even if the compiler does split this code into two reads of nn->nfsd_serv, it is guaranteed that the read value is always the same both times -- I guess that's that the comment is arguing, yes? I just wonder what will happen if that guarantee should happen to change in the future.
On Mon, 2025-01-27 at 09:03 -0500, Chuck Lever wrote: > On 1/27/25 8:39 AM, Chuck Lever wrote: > > On 1/27/25 8:32 AM, Jeff Layton wrote: > > > On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: > > > > On 1/27/25 8:07 AM, Jeff Layton wrote: > > > > > On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: > > > > > > On Mon, 27 Jan 2025, Jeff Layton wrote: > > > > > > > On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > > > > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > > > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > > > > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > > > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > > > > > > > > laundrette, which is shut down after the nfsd threads are shut > > > > > > > > > > > down and > > > > > > > > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL > > > > > > > > > > > then there > > > > > > > > > > > are no threads to wake. > > > > > > > > > > > > > > > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > > > > > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe > > > > > > > > > > > since the > > > > > > > > > > > svc_serv is not freed until after the filecache laundrette is > > > > > > > > > > > cancelled. > > > > > > > > > > > > > > > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files > > > > > > > > > > > to a work queue") > > > > > > > > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > > > > > > > > Closes: https://lore.kernel.org/linux- > > > > > > > > > > > nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > > > > > > --- > > > > > > > > > > > This is only lightly tested, but I think it will fix the bug that > > > > > > > > > > > Salvatore reported. > > > > > > > > > > > --- > > > > > > > > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > > > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > > > > > > > > index > > > > > > > > > > > e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > > > > > > > > --- a/fs/nfsd/filecache.c > > > > > > > > > > > +++ b/fs/nfsd/filecache.c > > > > > > > > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct > > > > > > > > > > > list_head *dispose) > > > > > > > > > > > struct nfsd_file, nf_gc); > > > > > > > > > > > struct nfsd_net *nn = net_generic(nf->nf_net, > > > > > > > > > > > nfsd_net_id); > > > > > > > > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > > > > > > > > + struct svc_serv *serv; > > > > > > > > > > > spin_lock(&l->lock); > > > > > > > > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > > > > > > > > spin_unlock(&l->lock); > > > > > > > > > > > - svc_wake_up(nn->nfsd_serv); > > > > > > > > > > > + > > > > > > > > > > > + /* > > > > > > > > > > > + * The filecache laundrette is shut down after the > > > > > > > > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > > > > > > > > + * svc_serv is freed. > > > > > > > > > > > + */ > > > > > > > > > > > + serv = nn->nfsd_serv; > > > > > > > > > > > > > > > > > > > > I wonder if this should be READ_ONCE() to tell the compiler > > > > > > > > > > that we > > > > > > > > > > could race with clearing nn->nfsd_serv. Would the comment > > > > > > > > > > still be > > > > > > > > > > needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we need a comment at least. The linkage between the > > > > > > > > > laundrette > > > > > > > > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > > > > > > > > doesn't convey that well, and is unnecessary here. > > > > > > > > > > > > > > > > Why do you say "is unnecessary here" ? > > > > > > > > If the code were > > > > > > > > if (nn->nfsd_serv) > > > > > > > > svc_wake_up(nn->nfsd_serv); > > > > > > > > that would be wrong as nn->nfds_serv could be set to NULL between > > > > > > > > the > > > > > > > > two. > > > > > > > > And the C compile is allowed to load the value twice because the > > > > > > > > C memory > > > > > > > > model declares that would have the same effect. > > > > > > > > While I doubt it would actually change how the code is compiled, > > > > > > > > I think > > > > > > > > we should have READ_ONCE() here (and I've been wrong before about > > > > > > > > what > > > > > > > > the compiler will actually do). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's unnecessary because the outcome of either case is acceptable. > > > > > > > > > > > > > > When racing with shutdown, either it's NULL and the laundrette won't > > > > > > > call svc_wake_up(), or it's non-NULL and it will. In the non-NULL > > > > > > > case, > > > > > > > the call to svc_wake_up() will be a no-op because the threads are > > > > > > > shut > > > > > > > down. > > > > > > > > > > > > > > The vastly common case in this code is that this pointer will be non- > > > > > > > NULL, because the server is running (i.e. not racing with > > > > > > > shutdown). I > > > > > > > don't see the need in making all of those accesses volatile. > > > > > > > > > > > > One of us is confused. I hope it isn't me. > > > > > > > > > > > > > > > > It's probably me. I think you have a much better understanding of > > > > > compiler design than I do. Still... > > > > > > > > > > > The hypothetical problem I see is that the C compiler could generate > > > > > > code to load the value "nn->nfsd_serv" twice. The first time it is > > > > > > not > > > > > > NULL, the second time it is NULL. > > > > > > The first is used for the test, the second is passed to svc_wake_up(). > > > > > > > > > > > > Unlikely though this is, it is possible and READ_ONCE() is designed > > > > > > precisely to prevent this. > > > > > > To quote from include/asm-generic/rwonce.h it will > > > > > > "Prevent the compiler from merging or refetching reads" > > > > > > > > > > > > A "volatile" access does not add any cost (in this case). What it > > > > > > does > > > > > > is break any aliasing that the compile might have deduced. > > > > > > Even if the compiler thinks it has "nn->nfsd_serv" in a register, it > > > > > > won't think it has the result of READ_ONCE(nn->nfsd_serv) in that > > > > > > register. > > > > > > And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it > > > > > > won't decide that it can just read nn->nfsd_serv again. It MUST keep > > > > > > the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not > > > > > > needed > > > > > > any more. > > > > > > > > > > I'm mainly just considering the resulting pointer. There are two > > > > > possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid > > > > > pointer that points to the svc_serv, or it's NULL. The resulting code > > > > > can handle either case, so it doesn't seem like adding READ_ONCE() will > > > > > create any material difference here. > > > > > > > > > > Maybe I should ask it this way: What bad outcome could result if we > > > > > don't add READ_ONCE() here? > > > > > > > > Neil just described it. The compiler would generate two load operations, > > > > one for the test and one for the function call argument. The first load > > > > can retrieve a non-NULL address, and the second a NULL address. > > > > > > > > I agree a READ_ONCE() is necessary. > > > > > > > > > > > > > > Now I'm confused: > > > > > > struct svc_serv *serv; > > > > > > [...] > > > > > > /* > > > * The filecache laundrette is shut down after the > > > * nn->nfsd_serv pointer is cleared, but before the > > > * svc_serv is freed. > > > */ > > > serv = nn->nfsd_serv; > > > if (serv) > > > svc_wake_up(serv); > > > > > > This code is explicitly asking to fetch nn->nfsd_serv into the serv > > > variable, and then is testing that copy of the pointer and passing it > > > into svc_wake_up(). > > > > > > How is the compiler allowed to suddenly refetch a NULL pointer into > > > serv after testing that serv is non-NULL? > > > > There's nothing here that tells the compiler it is not allowed to > > optimize this into two separate fetches if it feels that is better > > code. READ_ONCE is what tells the compiler we do not want that re- > > organization /ever/. > > > > > > Well, I think you can argue that even if the compiler does split this > code into two reads of nn->nfsd_serv, it is guaranteed that the read > value is always the same both times -- I guess that's that the comment > is arguing, yes? > Exactly. That's why I didn't just do: if (nn->nfsd_serv) svc_destroy(nn->nfsd_serv); We just have to ensure that we don't pass a NULL pointer to svc_destroy() and that should be guaranteed by fetching it into an interim variable. A READ_ONCE() doesn't buy us anything extra in this situation. It ensures that it doesn't use a cached version of nn->nfsd_serv when it does the fetch, but using a cached version is harmless here. Either way will still work. Plus, if this had access to a cached version of that variable in a register, it avoided a memory fetch. Given that this should almost never be NULL, adding READ_ONCE() seems like a waste. > I just wonder what will happen if that guarantee should happen to change > in the future. I think the only way this could break would be if nfsd_destroy_serv() started calling svc_destroy(&serv) before canceling the laundrette wq job. Maybe it's worth a comment in there pointing out that dependency.
On Tue, 28 Jan 2025, Jeff Layton wrote: > On Mon, 2025-01-27 at 09:03 -0500, Chuck Lever wrote: > > On 1/27/25 8:39 AM, Chuck Lever wrote: > > > On 1/27/25 8:32 AM, Jeff Layton wrote: > > > > On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: > > > > > On 1/27/25 8:07 AM, Jeff Layton wrote: > > > > > > On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: > > > > > > > On Mon, 27 Jan 2025, Jeff Layton wrote: > > > > > > > > On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > > > > > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > > > > > On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > > > > > > > > > > > On Sun, 26 Jan 2025, Jeff Layton wrote: > > > > > > > > > > > > nfsd_file_dispose_list_delayed can be called from the filecache > > > > > > > > > > > > laundrette, which is shut down after the nfsd threads are shut > > > > > > > > > > > > down and > > > > > > > > > > > > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL > > > > > > > > > > > > then there > > > > > > > > > > > > are no threads to wake. > > > > > > > > > > > > > > > > > > > > > > > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > > > > > > > > > > > > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe > > > > > > > > > > > > since the > > > > > > > > > > > > svc_serv is not freed until after the filecache laundrette is > > > > > > > > > > > > cancelled. > > > > > > > > > > > > > > > > > > > > > > > > Fixes: ffb402596147 ("nfsd: Don't leave work of closing files > > > > > > > > > > > > to a work queue") > > > > > > > > > > > > Reported-by: Salvatore Bonaccorso <carnil@debian.org> > > > > > > > > > > > > Closes: https://lore.kernel.org/linux- > > > > > > > > > > > > nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > > > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > > > > > > > --- > > > > > > > > > > > > This is only lightly tested, but I think it will fix the bug that > > > > > > > > > > > > Salvatore reported. > > > > > > > > > > > > --- > > > > > > > > > > > > fs/nfsd/filecache.c | 11 ++++++++++- > > > > > > > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > > > > > > > > > > > index > > > > > > > > > > > > e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > > > > > > > > > > > > --- a/fs/nfsd/filecache.c > > > > > > > > > > > > +++ b/fs/nfsd/filecache.c > > > > > > > > > > > > @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct > > > > > > > > > > > > list_head *dispose) > > > > > > > > > > > > struct nfsd_file, nf_gc); > > > > > > > > > > > > struct nfsd_net *nn = net_generic(nf->nf_net, > > > > > > > > > > > > nfsd_net_id); > > > > > > > > > > > > struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > > > > > > > > > > > + struct svc_serv *serv; > > > > > > > > > > > > spin_lock(&l->lock); > > > > > > > > > > > > list_move_tail(&nf->nf_gc, &l->freeme); > > > > > > > > > > > > spin_unlock(&l->lock); > > > > > > > > > > > > - svc_wake_up(nn->nfsd_serv); > > > > > > > > > > > > + > > > > > > > > > > > > + /* > > > > > > > > > > > > + * The filecache laundrette is shut down after the > > > > > > > > > > > > + * nn->nfsd_serv pointer is cleared, but before the > > > > > > > > > > > > + * svc_serv is freed. > > > > > > > > > > > > + */ > > > > > > > > > > > > + serv = nn->nfsd_serv; > > > > > > > > > > > > > > > > > > > > > > I wonder if this should be READ_ONCE() to tell the compiler > > > > > > > > > > > that we > > > > > > > > > > > could race with clearing nn->nfsd_serv. Would the comment > > > > > > > > > > > still be > > > > > > > > > > > needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we need a comment at least. The linkage between the > > > > > > > > > > laundrette > > > > > > > > > > and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > > > > > > > > > > doesn't convey that well, and is unnecessary here. > > > > > > > > > > > > > > > > > > Why do you say "is unnecessary here" ? > > > > > > > > > If the code were > > > > > > > > > if (nn->nfsd_serv) > > > > > > > > > svc_wake_up(nn->nfsd_serv); > > > > > > > > > that would be wrong as nn->nfds_serv could be set to NULL between > > > > > > > > > the > > > > > > > > > two. > > > > > > > > > And the C compile is allowed to load the value twice because the > > > > > > > > > C memory > > > > > > > > > model declares that would have the same effect. > > > > > > > > > While I doubt it would actually change how the code is compiled, > > > > > > > > > I think > > > > > > > > > we should have READ_ONCE() here (and I've been wrong before about > > > > > > > > > what > > > > > > > > > the compiler will actually do). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's unnecessary because the outcome of either case is acceptable. > > > > > > > > > > > > > > > > When racing with shutdown, either it's NULL and the laundrette won't > > > > > > > > call svc_wake_up(), or it's non-NULL and it will. In the non-NULL > > > > > > > > case, > > > > > > > > the call to svc_wake_up() will be a no-op because the threads are > > > > > > > > shut > > > > > > > > down. > > > > > > > > > > > > > > > > The vastly common case in this code is that this pointer will be non- > > > > > > > > NULL, because the server is running (i.e. not racing with > > > > > > > > shutdown). I > > > > > > > > don't see the need in making all of those accesses volatile. > > > > > > > > > > > > > > One of us is confused. I hope it isn't me. > > > > > > > > > > > > > > > > > > > It's probably me. I think you have a much better understanding of > > > > > > compiler design than I do. Still... > > > > > > > > > > > > > The hypothetical problem I see is that the C compiler could generate > > > > > > > code to load the value "nn->nfsd_serv" twice. The first time it is > > > > > > > not > > > > > > > NULL, the second time it is NULL. > > > > > > > The first is used for the test, the second is passed to svc_wake_up(). > > > > > > > > > > > > > > Unlikely though this is, it is possible and READ_ONCE() is designed > > > > > > > precisely to prevent this. > > > > > > > To quote from include/asm-generic/rwonce.h it will > > > > > > > "Prevent the compiler from merging or refetching reads" > > > > > > > > > > > > > > A "volatile" access does not add any cost (in this case). What it > > > > > > > does > > > > > > > is break any aliasing that the compile might have deduced. > > > > > > > Even if the compiler thinks it has "nn->nfsd_serv" in a register, it > > > > > > > won't think it has the result of READ_ONCE(nn->nfsd_serv) in that > > > > > > > register. > > > > > > > And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it > > > > > > > won't decide that it can just read nn->nfsd_serv again. It MUST keep > > > > > > > the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not > > > > > > > needed > > > > > > > any more. > > > > > > > > > > > > I'm mainly just considering the resulting pointer. There are two > > > > > > possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid > > > > > > pointer that points to the svc_serv, or it's NULL. The resulting code > > > > > > can handle either case, so it doesn't seem like adding READ_ONCE() will > > > > > > create any material difference here. > > > > > > > > > > > > Maybe I should ask it this way: What bad outcome could result if we > > > > > > don't add READ_ONCE() here? > > > > > > > > > > Neil just described it. The compiler would generate two load operations, > > > > > one for the test and one for the function call argument. The first load > > > > > can retrieve a non-NULL address, and the second a NULL address. > > > > > > > > > > I agree a READ_ONCE() is necessary. > > > > > > > > > > > > > > > > > > Now I'm confused: > > > > > > > > struct svc_serv *serv; > > > > > > > > [...] > > > > > > > > /* > > > > * The filecache laundrette is shut down after the > > > > * nn->nfsd_serv pointer is cleared, but before the > > > > * svc_serv is freed. > > > > */ > > > > serv = nn->nfsd_serv; > > > > if (serv) > > > > svc_wake_up(serv); > > > > > > > > This code is explicitly asking to fetch nn->nfsd_serv into the serv > > > > variable, and then is testing that copy of the pointer and passing it > > > > into svc_wake_up(). > > > > > > > > How is the compiler allowed to suddenly refetch a NULL pointer into > > > > serv after testing that serv is non-NULL? > > > > > > There's nothing here that tells the compiler it is not allowed to > > > optimize this into two separate fetches if it feels that is better > > > code. READ_ONCE is what tells the compiler we do not want that re- > > > organization /ever/. > > > > > > > > > > Well, I think you can argue that even if the compiler does split this > > code into two reads of nn->nfsd_serv, it is guaranteed that the read > > value is always the same both times -- I guess that's that the comment > > is arguing, yes? > > > > Exactly. That's why I didn't just do: > > if (nn->nfsd_serv) > svc_destroy(nn->nfsd_serv); > > We just have to ensure that we don't pass a NULL pointer to > svc_destroy() and that should be guaranteed by fetching it into an > interim variable. > > A READ_ONCE() doesn't buy us anything extra in this situation. It > ensures that it doesn't use a cached version of nn->nfsd_serv when it > does the fetch, but using a cached version is harmless here. Either way > will still work. Yes, READ_ONCE() ensures we don't use a cached version but, as you say, that is not relevant here. But that is not the ONLY thing that READ_ONCE() does. Remember what I quoted from rwonce.h: * Prevent the compiler from merging or refetching reads or writes. ^^^^^^^^^^^^^ There are 2 things that READ_ONCE() does. 1/ it avoids "merging" so a cached value won't be used 2/ it avoids "refetching". THAT is what we need to ensure. This is more practically relevant in larger functions. If I had serv = nn->nfsd_serv; if (serv) { /* do lots of other complicated stuff with other * fields from nn and probably call some functions, but * don't use serv or nn->nfsd_serv */ svc_destroy(serv); } then while compiling that complicated stuff the compiler might find that it wants to spill the register containing "serv". But doesn't want to spill "nn" because that is more actively used. So instead of spilling "serv" to the stack it just reuses the register and reloads nn->nfsd_serv for the final call. This is perfectly legal in C because the memory model affirms that memory won't spontaneously change unless it is marked "volatile" (or unless it is told that memory might have changed which is that "barrier()" does). In the particular code in question there isn't any complexity in between so I cannot imagine the compiler wanting to spill anything. But maybe one day the code will be changed. It is best-practice to use an appropriate accessor for data that might change in some other thread. Sometimes that means holding a lock, sometimes it mean atomic_read or rcu_dereference. Sometimes it means READ_ONCE(). Accessing data that can be changed by another thread without something like that is almost certainly "wrong". > > Plus, if this had access to a cached version of that variable in a > register, it avoided a memory fetch. Given that this should almost > never be NULL, adding READ_ONCE() seems like a waste. But it doesn't have access to a cached version. In this code, this is the first use of nn->nfsd_serv. So the READ_ONCE() can have no performance cost at all. If the code were ever changed to add more uses for nn->nfsd_serv, that READ_ONCE would serve as a loud reminder that the value can change and so any new accesses should happen after the READ_ONCE(). Thanks, NeilBrown
On Tue, 28 Jan 2025, Chuck Lever wrote: > On 1/27/25 8:39 AM, Chuck Lever wrote: > > On 1/27/25 8:32 AM, Jeff Layton wrote: > >> On Mon, 2025-01-27 at 08:22 -0500, Chuck Lever wrote: > >>> On 1/27/25 8:07 AM, Jeff Layton wrote: > >>>> On Mon, 2025-01-27 at 11:15 +1100, NeilBrown wrote: > >>>>> On Mon, 27 Jan 2025, Jeff Layton wrote: > >>>>>> On Mon, 2025-01-27 at 08:53 +1100, NeilBrown wrote: > >>>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: > >>>>>>>> On Sun, 2025-01-26 at 13:39 +1100, NeilBrown wrote: > >>>>>>>>> On Sun, 26 Jan 2025, Jeff Layton wrote: > >>>>>>>>>> nfsd_file_dispose_list_delayed can be called from the filecache > >>>>>>>>>> laundrette, which is shut down after the nfsd threads are shut > >>>>>>>>>> down and > >>>>>>>>>> the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL > >>>>>>>>>> then there > >>>>>>>>>> are no threads to wake. > >>>>>>>>>> > >>>>>>>>>> Ensure that the nn->nfsd_serv pointer is non-NULL before calling > >>>>>>>>>> svc_wake_up in nfsd_file_dispose_list_delayed. This is safe > >>>>>>>>>> since the > >>>>>>>>>> svc_serv is not freed until after the filecache laundrette is > >>>>>>>>>> cancelled. > >>>>>>>>>> > >>>>>>>>>> Fixes: ffb402596147 ("nfsd: Don't leave work of closing files > >>>>>>>>>> to a work queue") > >>>>>>>>>> Reported-by: Salvatore Bonaccorso <carnil@debian.org> > >>>>>>>>>> Closes: https://lore.kernel.org/linux- > >>>>>>>>>> nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ > >>>>>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> > >>>>>>>>>> --- > >>>>>>>>>> This is only lightly tested, but I think it will fix the bug that > >>>>>>>>>> Salvatore reported. > >>>>>>>>>> --- > >>>>>>>>>> fs/nfsd/filecache.c | 11 ++++++++++- > >>>>>>>>>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > >>>>>>>>>> index > >>>>>>>>>> e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 > >>>>>>>>>> --- a/fs/nfsd/filecache.c > >>>>>>>>>> +++ b/fs/nfsd/filecache.c > >>>>>>>>>> @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct > >>>>>>>>>> list_head *dispose) > >>>>>>>>>> struct nfsd_file, nf_gc); > >>>>>>>>>> struct nfsd_net *nn = net_generic(nf->nf_net, > >>>>>>>>>> nfsd_net_id); > >>>>>>>>>> struct nfsd_fcache_disposal *l = nn->fcache_disposal; > >>>>>>>>>> + struct svc_serv *serv; > >>>>>>>>>> spin_lock(&l->lock); > >>>>>>>>>> list_move_tail(&nf->nf_gc, &l->freeme); > >>>>>>>>>> spin_unlock(&l->lock); > >>>>>>>>>> - svc_wake_up(nn->nfsd_serv); > >>>>>>>>>> + > >>>>>>>>>> + /* > >>>>>>>>>> + * The filecache laundrette is shut down after the > >>>>>>>>>> + * nn->nfsd_serv pointer is cleared, but before the > >>>>>>>>>> + * svc_serv is freed. > >>>>>>>>>> + */ > >>>>>>>>>> + serv = nn->nfsd_serv; > >>>>>>>>> > >>>>>>>>> I wonder if this should be READ_ONCE() to tell the compiler > >>>>>>>>> that we > >>>>>>>>> could race with clearing nn->nfsd_serv. Would the comment > >>>>>>>>> still be > >>>>>>>>> needed? > >>>>>>>>> > >>>>>>>> > >>>>>>>> I think we need a comment at least. The linkage between the > >>>>>>>> laundrette > >>>>>>>> and the nfsd_serv being set to NULL is very subtle. A READ_ONCE() > >>>>>>>> doesn't convey that well, and is unnecessary here. > >>>>>>> > >>>>>>> Why do you say "is unnecessary here" ? > >>>>>>> If the code were > >>>>>>> if (nn->nfsd_serv) > >>>>>>> svc_wake_up(nn->nfsd_serv); > >>>>>>> that would be wrong as nn->nfds_serv could be set to NULL between > >>>>>>> the > >>>>>>> two. > >>>>>>> And the C compile is allowed to load the value twice because the > >>>>>>> C memory > >>>>>>> model declares that would have the same effect. > >>>>>>> While I doubt it would actually change how the code is compiled, > >>>>>>> I think > >>>>>>> we should have READ_ONCE() here (and I've been wrong before about > >>>>>>> what > >>>>>>> the compiler will actually do). > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> It's unnecessary because the outcome of either case is acceptable. > >>>>>> > >>>>>> When racing with shutdown, either it's NULL and the laundrette won't > >>>>>> call svc_wake_up(), or it's non-NULL and it will. In the non-NULL > >>>>>> case, > >>>>>> the call to svc_wake_up() will be a no-op because the threads are > >>>>>> shut > >>>>>> down. > >>>>>> > >>>>>> The vastly common case in this code is that this pointer will be non- > >>>>>> NULL, because the server is running (i.e. not racing with > >>>>>> shutdown). I > >>>>>> don't see the need in making all of those accesses volatile. > >>>>> > >>>>> One of us is confused. I hope it isn't me. > >>>>> > >>>> > >>>> It's probably me. I think you have a much better understanding of > >>>> compiler design than I do. Still... > >>>> > >>>>> The hypothetical problem I see is that the C compiler could generate > >>>>> code to load the value "nn->nfsd_serv" twice. The first time it is > >>>>> not > >>>>> NULL, the second time it is NULL. > >>>>> The first is used for the test, the second is passed to svc_wake_up(). > >>>>> > >>>>> Unlikely though this is, it is possible and READ_ONCE() is designed > >>>>> precisely to prevent this. > >>>>> To quote from include/asm-generic/rwonce.h it will > >>>>> "Prevent the compiler from merging or refetching reads" > >>>>> > >>>>> A "volatile" access does not add any cost (in this case). What it > >>>>> does > >>>>> is break any aliasing that the compile might have deduced. > >>>>> Even if the compiler thinks it has "nn->nfsd_serv" in a register, it > >>>>> won't think it has the result of READ_ONCE(nn->nfsd_serv) in that > >>>>> register. > >>>>> And if it needs the result of a previous READ_ONCE(nn->nfsd_serv) it > >>>>> won't decide that it can just read nn->nfsd_serv again. It MUST keep > >>>>> the result of READ_ONCE(nn->nfsd_serv) somewhere until it is not > >>>>> needed > >>>>> any more. > >>>> > >>>> I'm mainly just considering the resulting pointer. There are two > >>>> possible outcomes to the fetch of nn->nfsd_serv. Either it's a valid > >>>> pointer that points to the svc_serv, or it's NULL. The resulting code > >>>> can handle either case, so it doesn't seem like adding READ_ONCE() will > >>>> create any material difference here. > >>>> > >>>> Maybe I should ask it this way: What bad outcome could result if we > >>>> don't add READ_ONCE() here? > >>> > >>> Neil just described it. The compiler would generate two load operations, > >>> one for the test and one for the function call argument. The first load > >>> can retrieve a non-NULL address, and the second a NULL address. > >>> > >>> I agree a READ_ONCE() is necessary. > >>> > >>> > >> > >> Now I'm confused: > >> > >> struct svc_serv *serv; > >> > >> [...] > >> > >> /* > >> * The filecache laundrette is shut down after the > >> * nn->nfsd_serv pointer is cleared, but before the > >> * svc_serv is freed. > >> */ > >> serv = nn->nfsd_serv; > >> if (serv) > >> svc_wake_up(serv); > >> > >> This code is explicitly asking to fetch nn->nfsd_serv into the serv > >> variable, and then is testing that copy of the pointer and passing it > >> into svc_wake_up(). > >> > >> How is the compiler allowed to suddenly refetch a NULL pointer into > >> serv after testing that serv is non-NULL? > > > > There's nothing here that tells the compiler it is not allowed to > > optimize this into two separate fetches if it feels that is better > > code. READ_ONCE is what tells the compiler we do not want that re- > > organization /ever/. > > > > > > Well, I think you can argue that even if the compiler does split this > code into two reads of nn->nfsd_serv, it is guaranteed that the read > value is always the same both times -- I guess that's that the comment > is arguing, yes? I don't think that argument can stand. This code is run from the filecache laundrette which can still be active when nn->nfsd_serv is set to NULL. That is what the comment says: it is only shutdown *after* ->nfsd_serv is cleared. NeilBrown
From: Chuck Lever <chuck.lever@oracle.com> On Sat, 25 Jan 2025 20:13:18 -0500, Jeff Layton wrote: > nfsd_file_dispose_list_delayed can be called from the filecache > laundrette, which is shut down after the nfsd threads are shut down and > the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there > are no threads to wake. > > Ensure that the nn->nfsd_serv pointer is non-NULL before calling > svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the > svc_serv is not freed until after the filecache laundrette is cancelled. > > [...] Applied to nfsd-testing, thanks! Test experience should demonstrate whether more strict memory ordering semantics are needed. [1/1] nfsd: validate the nfsd_serv pointer before calling svc_wake_up commit: 363683ced1718d66ad54e1bdf52d41d544f783b2 -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index e91c164b5ea21507659904690533a19ca43b1b64..fb2a4469b7a3c077de2dd750f43239b4af6d37b0 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -445,11 +445,20 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) struct nfsd_file, nf_gc); struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); struct nfsd_fcache_disposal *l = nn->fcache_disposal; + struct svc_serv *serv; spin_lock(&l->lock); list_move_tail(&nf->nf_gc, &l->freeme); spin_unlock(&l->lock); - svc_wake_up(nn->nfsd_serv); + + /* + * The filecache laundrette is shut down after the + * nn->nfsd_serv pointer is cleared, but before the + * svc_serv is freed. + */ + serv = nn->nfsd_serv; + if (serv) + svc_wake_up(serv); } }
nfsd_file_dispose_list_delayed can be called from the filecache laundrette, which is shut down after the nfsd threads are shut down and the nfsd_serv pointer is cleared. If nn->nfsd_serv is NULL then there are no threads to wake. Ensure that the nn->nfsd_serv pointer is non-NULL before calling svc_wake_up in nfsd_file_dispose_list_delayed. This is safe since the svc_serv is not freed until after the filecache laundrette is cancelled. Fixes: ffb402596147 ("nfsd: Don't leave work of closing files to a work queue") Reported-by: Salvatore Bonaccorso <carnil@debian.org> Closes: https://lore.kernel.org/linux-nfs/7d9f2a8aede4f7ca9935a47e1d405643220d7946.camel@kernel.org/ Signed-off-by: Jeff Layton <jlayton@kernel.org> --- This is only lightly tested, but I think it will fix the bug that Salvatore reported. --- fs/nfsd/filecache.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) --- base-commit: 7541a5b8073cf0d9e2d288cac581f1aa6c11671d change-id: 20250125-kdevops-0989825ae8db Best regards,