On 2/4/25 8:53 PM, Li Lingfeng wrote:
>
> 在 2025/1/27 21:28, Chuck Lever 写道:
>> On 1/26/25 9:33 PM, Li Lingfeng wrote:
>>>
>>> 在 2025/1/27 1:27, Chuck Lever 写道:
>>>> On 1/26/25 4:50 AM, Li Lingfeng wrote:
>>>>> We got -ELOOP from ext4, resulting in the following WARNING:
>>>>>
>>>>> VFS: Lookup of 'dc' in ext4 sdd would have caused loop
>>>>> ------------[ cut here ]------------
>>>>> nfsd: non-standard errno: -40
>>>>> WARNING: CPU: 1 PID: 297024 at fs/nfsd/vfs.c:113 nfserrno+0xc8/0x128
>>>>> Modules linked in:
>>>>> CPU: 1 PID: 297024 Comm: nfsd Not tainted 6.6.0-gfa4c2159cd0d-dirty
>>>>> #21
>>>>> Hardware name: linux,dummy-virt (DT)
>>>>> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>> pc : nfserrno+0xc8/0x128
>>>>> lr : nfserrno+0xc8/0x128
>>>>> sp : ffff8000846475a0
>>>>> x29: ffff8000846475a0 x28: 0000000000000130 x27: ffff0000d65a24e8
>>>>> x26: ffff0000c7319134 x25: ffff0000d6de4240 x24: 0000000000000002
>>>>> x23: ffffcda9eaac3080 x22: 00000000ffffffd8 x21: 0000000000000026
>>>>> x20: ffffcda9ee055000 x19: 0000000000000000 x18: 0000000000000000
>>>>> x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>>>>> x14: 0000000000000000 x13: 0000000000000001 x12: ffff60001b5ca39b
>>>>> x11: 1fffe0001b5ca39a x10: ffff60001b5ca39a x9 : dfff800000000000
>>>>> x8 : 00009fffe4a35c66 x7 : ffff0000dae51cd3 x6 : 0000000000000001
>>>>> x5 : ffff0000dae51cd0 x4 : ffff60001b5ca39b x3 : dfff800000000000
>>>>> x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000ca5d8040
>>>>> Call trace:
>>>>> nfserrno+0xc8/0x128
>>>>> nfsd4_encode_dirent_fattr+0x358/0x380
>>>>> nfsd4_encode_dirent+0x164/0x3a8
>>>>> nfsd_buffered_readdir+0x1a8/0x3a0
>>>>> nfsd_readdir+0x14c/0x188
>>>>> nfsd4_encode_readdir+0x1d4/0x370
>>>>> nfsd4_encode_operation+0x130/0x518
>>>>> nfsd4_proc_compound+0x394/0xec0
>>>>> nfsd_dispatch+0x264/0x418
>>>>> svc_process_common+0x584/0xc78
>>>>> svc_process+0x1e8/0x2c0
>>>>> svc_recv+0x194/0x2d0
>>>>> nfsd+0x198/0x378
>>>>> kthread+0x1d8/0x1f0
>>>>> ret_from_fork+0x10/0x20
>>>>> Kernel panic - not syncing: kernel: panic_on_warn set ...
>>>>>
>>>>> The ELOOP error in Linux indicates that too many symbolic links were
>>>>> encountered in resolving a path name. Mapping it to nfserr_symlink
>>>>> may be
>>>>> fine.
>>>>>
>>>>> Signed-off-by: Li Lingfeng <lilingfeng3@xxxxxxxxxx>
>>>>> ---
>>>>> fs/nfsd/vfs.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>> index 29cb7b812d71..0f727010b8cb 100644
>>>>> --- a/fs/nfsd/vfs.c
>>>>> +++ b/fs/nfsd/vfs.c
>>>>> @@ -100,6 +100,7 @@ nfserrno (int errno)
>>>>> { nfserr_perm, -ENOKEY },
>>>>> { nfserr_no_grace, -ENOGRACE},
>>>>> { nfserr_io, -EBADMSG },
>>>>> + { nfserr_symlink, -ELOOP },
>>>>> };
>>>>> int i;
>>>>
>>>> Adding ELOOP -> SYMLINK as a generic mapping could be a problem.
>>>>
>>>> RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK as a permissible
>>>> status code for NFSv4 READDIR. Further, Section 15.4 lists only the
>>>> following operations for NFS4ERR_SYMLINK:
>>>>
>>>> COMMIT, LAYOUTCOMMIT, LINK, LOCK, LOCKT, LOOKUP, LOOKUPP, OPEN,
>>>> READ, WRITE
>>>>
>>>>
>>>> Which of lookup_positive_unlocked() or nfsd_cross_mnt() returned
>>>> ELOOP, and why? What were the export options? What was in the file
>>>> system that caused this? Can this scenario be reproduced on v6.13?
>>>>
>>> Hi,
>>> I got a more detailed log with line numbers from our test team.
>>>
>>> VFS: Lookup of 'dc' in ext4 sdd would have caused loop
>>> ------------[ cut here ]------------
>>> nfsd: non-standard errno: -40
>>> WARNING: CPU: 1 PID: 297024 at fs/nfsd/vfs.c:113 nfserrno fs/nfsd/
>>> vfs.c:113 [inline]
>>> WARNING: CPU: 1 PID: 297024 at fs/nfsd/vfs.c:113 nfserrno+0xc8/0x128
>>> fs/ nfsd/vfs.c:61
>>> Modules linked in:
>>> CPU: 1 PID: 297024 Comm: nfsd Not tainted 6.6.0-gfa4c2159cd0d-dirty #21
>>> Hardware name: linux,dummy-virt (DT)
>>> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>> pc : nfserrno fs/nfsd/vfs.c:113 [inline]
>>> pc : nfserrno+0xc8/0x128 fs/nfsd/vfs.c:61
>>> lr : nfserrno fs/nfsd/vfs.c:113 [inline]
>>> lr : nfserrno+0xc8/0x128 fs/nfsd/vfs.c:61
>>> sp : ffff8000846475a0
>>> x29: ffff8000846475a0 x28: 0000000000000130 x27: ffff0000d65a24e8
>>> x26: ffff0000c7319134 x25: ffff0000d6de4240 x24: 0000000000000002
>>> x23: ffffcda9eaac3080 x22: 00000000ffffffd8 x21: 0000000000000026
>>> x20: ffffcda9ee055000 x19: 0000000000000000 x18: 0000000000000000
>>> x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>>> x14: 0000000000000000 x13: 0000000000000001 x12: ffff60001b5ca39b
>>> x11: 1fffe0001b5ca39a x10: ffff60001b5ca39a x9 : dfff800000000000
>>> x8 : 00009fffe4a35c66 x7 : ffff0000dae51cd3 x6 : 0000000000000001
>>> x5 : ffff0000dae51cd0 x4 : ffff60001b5ca39b x3 : dfff800000000000
>>> x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000ca5d8040
>>> Call trace:
>>> nfserrno fs/nfsd/vfs.c:113 [inline]
>>> nfserrno+0xc8/0x128 fs/nfsd/vfs.c:61
>>> nfsd4_encode_dirent_fattr+0x358/0x380 fs/nfsd/nfs4xdr.c:3536
>>> nfsd4_encode_dirent+0x164/0x3a8 fs/nfsd/nfs4xdr.c:3633
>>> nfsd_buffered_readdir+0x1a8/0x3a0 fs/nfsd/vfs.c:2067
>>> nfsd_readdir+0x14c/0x188 fs/nfsd/vfs.c:2123
>>> nfsd4_encode_readdir+0x1d4/0x370 fs/nfsd/nfs4xdr.c:4273
>>> nfsd4_encode_operation+0x130/0x518 fs/nfsd/nfs4xdr.c:5399
>>> nfsd4_proc_compound+0x394/0xec0 fs/nfsd/nfs4proc.c:2753
>>> nfsd_dispatch+0x264/0x418 fs/nfsd/nfssvc.c:1011
>>> svc_process_common+0x584/0xc78 net/sunrpc/svc.c:1396
>>> svc_process+0x1e8/0x2c0 net/sunrpc/svc.c:1542
>>> svc_recv+0x194/0x2d0 net/sunrpc/svc_xprt.c:877
>>> nfsd+0x198/0x378 fs/nfsd/nfssvc.c:955
>>> kthread+0x1d8/0x1f0 kernel/kthread.c:388
>>> ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:861
>>>
>>> Although I don't have a reproducer to reproduce this problem, I think
>>> ELOOP should be returned by the following path:
>>>
>>> v6.6
>>> nfsd4_encode_readdir
>>> nfsd_readdir
>>> nfsd_buffered_readdir
>>> nfsd4_encode_dirent // func
>>> nfsd4_encode_dirent_fattr
>>> nfsd4_encode_dirent_fattr
>>> lookup_positive_unlocked
>>> lookup_one_positive_unlocked
>>> lookup_one_unlocked // ELOOP
>>> lookup_slow
>>> __lookup_slow
>>> ext4_lookup // inode->i_op->lookup
>>> d_splice_alias
>>> // VFS: Lookup of 'dc' in ext4 sdd would have caused loop
>>>
>>> This scenario may be reproduced on v6.13 like this:
>>> nfsd4_encode_readdir
>>> nfsd4_encode_dirlist4
>>> nfsd_readdir
>>> nfsd_buffered_readdir
>>> nfsd4_encode_entry4 // func
>>> nfsd4_encode_entry4_fattr
>>> lookup_positive_unlocked
>>> lookup_one_positive_unlocked
>>> lookup_one_unlocked
>>> lookup_slow
>>> __lookup_slow
>>> ext4_lookup // inode->i_op->lookup
>>> d_splice_alias
>>
>> So: lookup_positive_unlocked() is the VFS API returning it. Got it.
>>
>>
>>> According to the information provided by the test team, the export
>>> option
>>> is "rw,no_root_squash", and I'll try to reproduce the problem.
>>>
>>> By the way, could you suggest which NFS error code would be most
>>> appropriate to map ELOOP to?
>>
>> NFS4ERR_SYMLINK is closest. But the spec says, you can't return that
>> status for every operation; in particular, READDIR does not allow it.
>> So I'm quite hesitant to correct the crash you found by adding this
>> mapping to nfserrno.
>>
>> In this case, I wonder if READDIR can simply not return attributes
>> when it hits an error.
Turns out, no: the spec has (non-normative) language that READDIR has
to fail in this case.
> Do you mean to add an ELOOP check like the following and return nfs_ok
> directly?
I wasn't thinking of special treatment for ELOOP. I am concerned about
NFSD returning NFS4ERR_SYMLINK as the status for a READDIR operation,
which the protocol spec forbids.
It's kind of interesting that there hasn't been a need to add an ELOOP
mapping to nfserrno() until now. I'm a little hesitant to add a generic
mapping without checking the thousand other places nfserrno() is called,
but that might end up being a necessary part of this fix.
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e67420729ecd..3a03eba9d4aa 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3814,7 +3814,7 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir
> *cd, const char *name,
>
> dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry,
> namlen);
> if (IS_ERR(dentry))
> - return nfserrno(PTR_ERR(dentry));
> + return (PTR_ERR(dentry) == -ELOOP) ? nfs_ok :
> nfserrno(PTR_ERR(dentry));
>
> exp_get(exp);
> /*
>
> I think it's a little weird to make this change just for ELOOP.
No doubt, but let's have a look at some code. The code in question is in
nfsd4_encode_entry4_fattr's caller:
nfserr = nfsd4_encode_entry4_fattr(cd, name, namlen);
switch (nfserr) {
...
default:
/*
* If the client requested the RDATTR_ERROR attribute,
* we stuff the error code into this attribute
* and continue. If this attribute was not requested,
* then in accordance with the spec, we fail the
* entire READDIR operation(!)
*/
if (!(cd->rd_bmval[0] & FATTR4_WORD0_RDATTR_ERROR))
goto fail;
if (nfsd4_encode_entry4_rdattr_error(xdr, nfserr)) {
nfserr = nfserr_toosmall;
goto fail;
}
}
...
fail:
xdr_truncate_encode(xdr, start_offset);
cd->common.err = nfserr;
return -EINVAL;
}
Not shown: if nfsd4_encode_entry4() returns a status code != nfs4_ok,
the current implementation packages that status value as the status code
for READDIR (when the client hasn't requested RDATTR_ERROR). The
default: arm shown above is where nfserr_symlink might leak.
I can't find any spec restrictions on the status code returned in an
RDATTR_ERROR attribute. Thus I believe setting the value of that
attribute to NFS4ERR_SYMLINK is permissible.
However, by RFC 8881 Section 15.2, READDIR is permitted to return:
NFS4ERR_ACCESS, NFS4ERR_BADXDR, NFS4ERR_BAD_COOKIE, NFS4ERR_DEADSESSION,
NFS4ERR_DELAY, NFS4ERR_FHEXPIRED, NFS4ERR_INVAL, NFS4ERR_IO,
NFS4ERR_MOVED, NFS4ERR_NOFILEHANDLE, NFS4ERR_NOTDIR, NFS4ERR_NOT_SAME,
NFS4ERR_OP_NOT_IN_SESSION, NFS4ERR_REP_TOO_BIG,
NFS4ERR_REP_TOO_BIG_TO_CACHE, NFS4ERR_REQ_TOO_BIG,
NFS4ERR_RETRY_UNCACHED_REP, NFS4ERR_SERVERFAULT, NFS4ERR_STALE,
NFS4ERR_TOOSMALL, NFS4ERR_TOO_MANY_OPS
So, if the client has not asserted FATTR4_WORD0_RDATTR_ERROR, NFSD
should set @nfserr to, say, nfserr_io in the default: arm before it goes
to "fail:" because READDIR mustn't leak arbitrary NFS4ERR values as its
status code.
Can you confirm my analysis via a network capture?
--
Chuck Lever
