> On Aug 23, 2024, at 11:03 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> On Fri, Aug 23, 2024 at 10:44 AM Chuck Lever <chuck.lever@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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
