| Message ID | 20240823141401.71740-1-okorniev@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] nfsd: prevent states_show() from using invalid stateids | expand |
On Fri, 2024-08-23 at 10:14 -0400, Olga Kornievskaia wrote: > states_show() relied on sc_type field to be of valid type > before calling into a subfunction to show content of a > particular stateid. But from commit 3f29cc82a84c we > split the validity of the stateid into sc_status and no longer > changed sc_type to 0 while unhashing the stateid. This > resulted in kernel oopsing as something like > nfs4_show_open() would derefence sc_file which was NULL. > > To reproduce: mount the server with 4.0, read and close > a file and then on the server cat /proc/fs/nfsd/clients/2/states > > [ 513.590804] Call trace: > [ 513.590925] _raw_spin_lock+0xcc/0x160 > [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] > [ 513.591412] states_show+0x44c/0x488 [nfsd] > [ 513.591681] seq_read_iter+0x5d8/0x760 > [ 513.591896] seq_read+0x188/0x208 > [ 513.592075] vfs_read+0x148/0x470 > [ 513.592241] ksys_read+0xcc/0x178 > > Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > fs/nfsd/nfs4state.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index c3def49074a4..8351724b8a43 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) > { > struct nfs4_stid *st = v; > > + if (!st->sc_file) > + return 0; > + > switch (st->sc_type) { > case SC_TYPE_OPEN: > return nfs4_show_open(s, st); OPEN stateids are the only ones that stick around for a while after we've closed them out (and that's only for v4.0, IIRC). The others all only release the file after unhashing. I think it'd be better to still display that stateid, and just not do any of the bits in nfs4_show_open that require ->sc_file. We have the "admin-revoked" string that we display now when SC_STATUS_ADMIN_REVOKED is set. Maybe we can display "closed" or something in this case?
On Fri, Aug 23, 2024 at 10:14:01AM -0400, Olga Kornievskaia wrote: > states_show() relied on sc_type field to be of valid type > before calling into a subfunction to show content of a > particular stateid. But from commit 3f29cc82a84c we > split the validity of the stateid into sc_status and no longer > changed sc_type to 0 while unhashing the stateid. This > resulted in kernel oopsing as something like > nfs4_show_open() would derefence sc_file which was NULL. > > To reproduce: mount the server with 4.0, read and close > a file and then on the server cat /proc/fs/nfsd/clients/2/states > > [ 513.590804] Call trace: > [ 513.590925] _raw_spin_lock+0xcc/0x160 > [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] > [ 513.591412] states_show+0x44c/0x488 [nfsd] > [ 513.591681] seq_read_iter+0x5d8/0x760 > [ 513.591896] seq_read+0x188/0x208 > [ 513.592075] vfs_read+0x148/0x470 > [ 513.592241] ksys_read+0xcc/0x178 > > Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > fs/nfsd/nfs4state.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index c3def49074a4..8351724b8a43 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) > { > struct nfs4_stid *st = v; > > + if (!st->sc_file) > + return 0; > + > switch (st->sc_type) { > case SC_TYPE_OPEN: > return nfs4_show_open(s, st); > -- > 2.43.5 > I'll wait for Neil/Jeff's Reviewed-by, but the root cause analysis seems plausible to me, and I'll plan to apply it for v6.11-rc. Btw, I noticed this at the tail of states_show(): /* XXX: copy stateids? */ I'm not really sure, but those won't ever have an sc_file, will they? Are COPY callback stateids something we want the server to display in this code? Just musing aloud: maybe the NULL sc_file check wants to be moved into the show helpers.
On Fri, Aug 23, 2024 at 10:40 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-08-23 at 10:14 -0400, Olga Kornievskaia wrote: > > states_show() relied on sc_type field to be of valid type > > before calling into a subfunction to show content of a > > particular stateid. But from commit 3f29cc82a84c we > > split the validity of the stateid into sc_status and no longer > > changed sc_type to 0 while unhashing the stateid. This > > resulted in kernel oopsing as something like > > nfs4_show_open() would derefence sc_file which was NULL. > > > > To reproduce: mount the server with 4.0, read and close > > a file and then on the server cat /proc/fs/nfsd/clients/2/states > > > > [ 513.590804] Call trace: > > [ 513.590925] _raw_spin_lock+0xcc/0x160 > > [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] > > [ 513.591412] states_show+0x44c/0x488 [nfsd] > > [ 513.591681] seq_read_iter+0x5d8/0x760 > > [ 513.591896] seq_read+0x188/0x208 > > [ 513.592075] vfs_read+0x148/0x470 > > [ 513.592241] ksys_read+0xcc/0x178 > > > > Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > --- > > fs/nfsd/nfs4state.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index c3def49074a4..8351724b8a43 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) > > { > > struct nfs4_stid *st = v; > > > > + if (!st->sc_file) > > + return 0; > > + > > switch (st->sc_type) { > > case SC_TYPE_OPEN: > > return nfs4_show_open(s, st); > > OPEN stateids are the only ones that stick around for a while after > we've closed them out (and that's only for v4.0, IIRC). The others all > only release the file after unhashing. That's good to know. I had a reproducer for the open but the locking stateid seem to be not affected by it (and I dont know yet how to setup a pnfs server). If only nfs4_show_open() needs to be fixed, I can do that. > I think it'd be better to still display that stateid, and just not do > any of the bits in nfs4_show_open that require ->sc_file. We have the > "admin-revoked" string that we display now when SC_STATUS_ADMIN_REVOKED > is set. Maybe we can display "closed" or something in this case? I'll do as you suggest and then add "closed" if sc_file is null. Thank you. > -- > Jeff Layton <jlayton@kernel.org> >
On Fri, Aug 23, 2024 at 10:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > On Fri, Aug 23, 2024 at 10:14:01AM -0400, Olga Kornievskaia wrote: > > states_show() relied on sc_type field to be of valid type > > before calling into a subfunction to show content of a > > particular stateid. But from commit 3f29cc82a84c we > > split the validity of the stateid into sc_status and no longer > > changed sc_type to 0 while unhashing the stateid. This > > resulted in kernel oopsing as something like > > nfs4_show_open() would derefence sc_file which was NULL. > > > > To reproduce: mount the server with 4.0, read and close > > a file and then on the server cat /proc/fs/nfsd/clients/2/states > > > > [ 513.590804] Call trace: > > [ 513.590925] _raw_spin_lock+0xcc/0x160 > > [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] > > [ 513.591412] states_show+0x44c/0x488 [nfsd] > > [ 513.591681] seq_read_iter+0x5d8/0x760 > > [ 513.591896] seq_read+0x188/0x208 > > [ 513.592075] vfs_read+0x148/0x470 > > [ 513.592241] ksys_read+0xcc/0x178 > > > > Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > --- > > fs/nfsd/nfs4state.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index c3def49074a4..8351724b8a43 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) > > { > > struct nfs4_stid *st = v; > > > > + if (!st->sc_file) > > + return 0; > > + > > switch (st->sc_type) { > > case SC_TYPE_OPEN: > > return nfs4_show_open(s, st); > > -- > > 2.43.5 > > > > I'll wait for Neil/Jeff's Reviewed-by, but the root cause analysis > seems plausible to me, and I'll plan to apply it for v6.11-rc. > > Btw, I noticed this at the tail of states_show(): > > /* XXX: copy stateids? */ > > I'm not really sure, but those won't ever have an sc_file, will > they? Are COPY callback stateids something we want the server to > display in this code? Just musing aloud: maybe the NULL sc_file > check wants to be moved into the show helpers. I can see that the copy stateids still have type NFS4_COPY_STID so they are not converted to the new type of SC_TYPE. But it just means we are not displaying them. I'm not sure what happens to sc_file for copy stateid, I'd need to check. > > -- > Chuck Lever >
> On Aug 23, 2024, at 11:03 AM, Olga Kornievskaia <aglo@umich.edu> wrote: > > On Fri, Aug 23, 2024 at 10:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On Fri, Aug 23, 2024 at 10:14:01AM -0400, Olga Kornievskaia wrote: >>> states_show() relied on sc_type field to be of valid type >>> before calling into a subfunction to show content of a >>> particular stateid. But from commit 3f29cc82a84c we >>> split the validity of the stateid into sc_status and no longer >>> changed sc_type to 0 while unhashing the stateid. This >>> resulted in kernel oopsing as something like >>> nfs4_show_open() would derefence sc_file which was NULL. >>> >>> To reproduce: mount the server with 4.0, read and close >>> a file and then on the server cat /proc/fs/nfsd/clients/2/states >>> >>> [ 513.590804] Call trace: >>> [ 513.590925] _raw_spin_lock+0xcc/0x160 >>> [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] >>> [ 513.591412] states_show+0x44c/0x488 [nfsd] >>> [ 513.591681] seq_read_iter+0x5d8/0x760 >>> [ 513.591896] seq_read+0x188/0x208 >>> [ 513.592075] vfs_read+0x148/0x470 >>> [ 513.592241] ksys_read+0xcc/0x178 >>> >>> Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>> --- >>> fs/nfsd/nfs4state.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index c3def49074a4..8351724b8a43 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) >>> { >>> struct nfs4_stid *st = v; >>> >>> + if (!st->sc_file) >>> + return 0; >>> + >>> switch (st->sc_type) { >>> case SC_TYPE_OPEN: >>> return nfs4_show_open(s, st); >>> -- >>> 2.43.5 >>> >> >> I'll wait for Neil/Jeff's Reviewed-by, but the root cause analysis >> seems plausible to me, and I'll plan to apply it for v6.11-rc. >> >> Btw, I noticed this at the tail of states_show(): >> >> /* XXX: copy stateids? */ >> >> I'm not really sure, but those won't ever have an sc_file, will >> they? Are COPY callback stateids something we want the server to >> display in this code? Just musing aloud: maybe the NULL sc_file >> check wants to be moved into the show helpers. > > I can see that the copy stateids still have type NFS4_COPY_STID so > they are not converted to the new type of SC_TYPE. But it just means > we are not displaying them. I'm not sure what happens to sc_file for > copy stateid, I'd need to check. If you're already going to move the sc_file check into nfs4_show_open(), you can defer looking at adding specific support for callback stateids. We'll add it to the to-do list. -- Chuck Lever
On Fri, Aug 23, 2024 at 11:10 AM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > On Aug 23, 2024, at 11:03 AM, Olga Kornievskaia <aglo@umich.edu> wrote: > > > > On Fri, Aug 23, 2024 at 10:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > >> > >> On Fri, Aug 23, 2024 at 10:14:01AM -0400, Olga Kornievskaia wrote: > >>> states_show() relied on sc_type field to be of valid type > >>> before calling into a subfunction to show content of a > >>> particular stateid. But from commit 3f29cc82a84c we > >>> split the validity of the stateid into sc_status and no longer > >>> changed sc_type to 0 while unhashing the stateid. This > >>> resulted in kernel oopsing as something like > >>> nfs4_show_open() would derefence sc_file which was NULL. > >>> > >>> To reproduce: mount the server with 4.0, read and close > >>> a file and then on the server cat /proc/fs/nfsd/clients/2/states > >>> > >>> [ 513.590804] Call trace: > >>> [ 513.590925] _raw_spin_lock+0xcc/0x160 > >>> [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] > >>> [ 513.591412] states_show+0x44c/0x488 [nfsd] > >>> [ 513.591681] seq_read_iter+0x5d8/0x760 > >>> [ 513.591896] seq_read+0x188/0x208 > >>> [ 513.592075] vfs_read+0x148/0x470 > >>> [ 513.592241] ksys_read+0xcc/0x178 > >>> > >>> Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") > >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > >>> --- > >>> fs/nfsd/nfs4state.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > >>> index c3def49074a4..8351724b8a43 100644 > >>> --- a/fs/nfsd/nfs4state.c > >>> +++ b/fs/nfsd/nfs4state.c > >>> @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) > >>> { > >>> struct nfs4_stid *st = v; > >>> > >>> + if (!st->sc_file) > >>> + return 0; > >>> + > >>> switch (st->sc_type) { > >>> case SC_TYPE_OPEN: > >>> return nfs4_show_open(s, st); > >>> -- > >>> 2.43.5 > >>> > >> > >> I'll wait for Neil/Jeff's Reviewed-by, but the root cause analysis > >> seems plausible to me, and I'll plan to apply it for v6.11-rc. > >> > >> Btw, I noticed this at the tail of states_show(): > >> > >> /* XXX: copy stateids? */ > >> > >> I'm not really sure, but those won't ever have an sc_file, will > >> they? Are COPY callback stateids something we want the server to > >> display in this code? Just musing aloud: maybe the NULL sc_file > >> check wants to be moved into the show helpers. > > > > I can see that the copy stateids still have type NFS4_COPY_STID so > > they are not converted to the new type of SC_TYPE. But it just means > > we are not displaying them. I'm not sure what happens to sc_file for > > copy stateid, I'd need to check. > > If you're already going to move the sc_file check into > nfs4_show_open(), you can defer looking at adding specific > support for callback stateids. We'll add it to the to-do > list. Now that I've looked at some copy code and also got more familiar with this walking the stateids in proc, I'd like to note that proc only walks the stateids that are store in clp->cl_stateids but the copy stateid that we keep around (and those are the copy stateids in the source server) are stored under the nfsd_net (namespace). Not to say we shouldn't modify states_show() not to add walking a different list but wanted to raise it. Also on the destination server, we stop async copy stateid under the clp->async_copies list and they only live for the lifetime of the copy. I know this bordes on a different discussion of keeping copy stateids... > > > -- > Chuck Lever > >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index c3def49074a4..8351724b8a43 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -2907,6 +2907,9 @@ static int states_show(struct seq_file *s, void *v) { struct nfs4_stid *st = v; + if (!st->sc_file) + return 0; + switch (st->sc_type) { case SC_TYPE_OPEN: return nfs4_show_open(s, st);
states_show() relied on sc_type field to be of valid type before calling into a subfunction to show content of a particular stateid. But from commit 3f29cc82a84c we split the validity of the stateid into sc_status and no longer changed sc_type to 0 while unhashing the stateid. This resulted in kernel oopsing as something like nfs4_show_open() would derefence sc_file which was NULL. To reproduce: mount the server with 4.0, read and close a file and then on the server cat /proc/fs/nfsd/clients/2/states [ 513.590804] Call trace: [ 513.590925] _raw_spin_lock+0xcc/0x160 [ 513.591119] nfs4_show_open+0x78/0x2c0 [nfsd] [ 513.591412] states_show+0x44c/0x488 [nfsd] [ 513.591681] seq_read_iter+0x5d8/0x760 [ 513.591896] seq_read+0x188/0x208 [ 513.592075] vfs_read+0x148/0x470 [ 513.592241] ksys_read+0xcc/0x178 Fixes: 3f29cc82a84c ("nfsd: split sc_status out of sc_type") Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> --- fs/nfsd/nfs4state.c | 3 +++ 1 file changed, 3 insertions(+)