[PATCH 0/2] nfsd: add a new mapping and remove the redundant one

[PATCH 0/2] nfsd: add a new mapping and remove the redundant one

[Li Lingfeng]

Add a new mapping and delete a redundant one.

Li Lingfeng (2):
  nfsd: map the ELOOP to nfserr_symlink to avoid warning
  nfsd: remove the redundant mapping of nfserr_mlink

 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[Li Lingfeng]

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@huawei.com>
---
 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;
 
-- 
2.31.1

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[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@huawei.com>
> ---
>   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?


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[Li Lingfeng]

在 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@huawei.com>
>> ---
>>   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

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?

Thanks.

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[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@huawei.com>
>>> ---
>>>   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.


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[Li Lingfeng]

在 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@huawei.com>
>>>> ---
>>>>   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.

Hi,

Do you mean to add an ELOOP check like the following and return nfs_ok
directly?

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.

Thanks.

>
>

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[Chuck Lever]

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@huawei.com>
>>>>> ---
>>>>>   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

Re: [PATCH 1/2] nfsd: map the ELOOP to nfserr_symlink to avoid warning

[NeilBrown]

On Thu, 06 Feb 2025, Chuck Lever wrote:
> 
> 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.


This ELOOP error is surprising on a local filesystem.  It means that the
lookup of the given name found an inode for a directory which already
existed in the dcache as an ancestor of the directory being listed - or
possibly as the directory itself.  For ext4, that means a corrupt
filesystem.

If the exported filesystem was NFS, then I think it is credible that a
complex race could result in this.

So we certainly need to handle ELOOP cleanly but we don't need to try
too hard to find a perfect solution.  Returning nfserr_io would be
defensible.  Nothing else really suits, certainly not NFS4ERR_SYMLINK
because there is no symlink involved here.

NeilBrown

[PATCH 2/2] nfsd: remove the redundant mapping of nfserr_mlink

[Li Lingfeng]

There two mappings of nfserr_mlink in nfs_errtbl.
Remove one of them.

Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
 fs/nfsd/vfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 0f727010b8cb..42039cff2124 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -71,7 +71,6 @@ nfserrno (int errno)
 		{ nfserr_acces, -EACCES },
 		{ nfserr_exist, -EEXIST },
 		{ nfserr_xdev, -EXDEV },
-		{ nfserr_mlink, -EMLINK },
 		{ nfserr_nodev, -ENODEV },
 		{ nfserr_notdir, -ENOTDIR },
 		{ nfserr_isdir, -EISDIR },
-- 
2.31.1

Re: [PATCH 0/2] nfsd: add a new mapping and remove the redundant one

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

On Sun, 26 Jan 2025 17:50:43 +0800, Li Lingfeng wrote:
> Add a new mapping and delete a redundant one.
> 
> Li Lingfeng (2):
>   nfsd: map the ELOOP to nfserr_symlink to avoid warning
>   nfsd: remove the redundant mapping of nfserr_mlink
> 
> fs/nfsd/vfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> [...]

Applied 2/2 to nfsd-testing, thanks!

Note that the address kolga@netapp.com is no longer in service.
Please do note include that address when posting NFSD patches.

[2/2] nfsd: remove the redundant mapping of nfserr_mlink
      commit: 6e6e2d282d6b18e629f35688a658a1e454dc3640

--
Chuck Lever

Re: [PATCH 0/2] nfsd: add a new mapping and remove the redundant one

[Jeff Layton]

On Sun, 2025-01-26 at 17:50 +0800, Li Lingfeng wrote:
> Add a new mapping and delete a redundant one.
> 
> Li Lingfeng (2):
>   nfsd: map the ELOOP to nfserr_symlink to avoid warning
>   nfsd: remove the redundant mapping of nfserr_mlink
> 
>  fs/nfsd/vfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>