| Message ID | 20241225065908.1547645-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfsd/sunrpc: cleanup resource with sync mode | expand |
From: Chuck Lever <chuck.lever@oracle.com> On Wed, 25 Dec 2024 14:59:04 +0800, Yang Erkun wrote: > After f8c989a0c89a ("nfsd: release svc_expkey/svc_export with > rcu_work"), svc_export_put/expkey_put will call path_put with async > mode. This can lead some unexpected failure: > > mkdir /mnt/sda > mkfs.xfs -f /dev/sda > echo "/ *(rw,no_root_squash,fsid=0)" > /etc/exports > echo "/mnt *(rw,no_root_squash,fsid=1)" >> /etc/exports > exportfs -ra > service nfs-server start > mount -t nfs -o vers=4.0 127.0.0.1:/mnt /mnt1 > mount /dev/sda /mnt/sda > touch /mnt1/sda/file > exportfs -r > umount /mnt/sda # failed unexcepted > > [...] Applied to nfsd-testing for v6.14, thanks! I'm unsure whether 2/4 and 3/4 need Fixes: tags. Based on the patch descriptions, these appear to be pre-requisite patches rather than actual fixes. However, I'm guessing that 4/4 will apply and build without 2/4 and 3/4 applied, but there will be operational problems. So explicit Fixes: tags in 2/4 and 3/4 are probably necessary to get a properly working backport. This is a case where I would rather have manual backporting with a full CI run rather than just slapping these patches on the LTS kernels and hoping for the best. [1/4] SUNRPC: introduce cache_check_rcu to help check in rcu context commit: 401fd94cb21c978403077d611bdc311dc8376f9d [2/4] nfsd: no need get cache ref when protected by rcu commit: 3d32486aa35bf4b99edbfc27538b24fadebd7f75 [3/4] SUNRPC: no need get cache ref when protected by rcu commit: 9f5fc67903b5709c4334bf33764d6fbf0ceb05b4 [4/4] nfsd: fix UAF when access ex_uuid or ex_stats commit: 8fa5adbee5ab904ea282da183ece664a67db2b2b -- Chuck Lever
On Wed, 2024-12-25 at 14:59 +0800, Yang Erkun wrote: > After f8c989a0c89a ("nfsd: release svc_expkey/svc_export with > rcu_work"), svc_export_put/expkey_put will call path_put with async > mode. This can lead some unexpected failure: > > mkdir /mnt/sda > mkfs.xfs -f /dev/sda > echo "/ *(rw,no_root_squash,fsid=0)" > /etc/exports > echo "/mnt *(rw,no_root_squash,fsid=1)" >> /etc/exports > exportfs -ra > service nfs-server start > mount -t nfs -o vers=4.0 127.0.0.1:/mnt /mnt1 > mount /dev/sda /mnt/sda > touch /mnt1/sda/file > exportfs -r > umount /mnt/sda # failed unexcepted > > The touch above will finally call nfsd_cross_mnt, add refcount to mount, > and then add cache_head. Before this commit, exportfs -r will call > cache_flush to cleanup all cache_head, and path_put in > svc_export_put/expkey_put will be finished with sync mode. So, the > latter umount will always success. However, after this commit, path_put > will be called with async mode, the latter umount may failed, and if we > add some delay, umount will success too. Personally I think this bug and > should be fixed. We first revert before bugfix patch, and then fix the > original bug with a different way. > > v1->v2: > > 1. do not change cache_check, instead add cache_check_rcu and use it in > c_show/e_show > 2. the first patch has been applied > > Yang Erkun (4): > SUNRPC: introduce cache_check_rcu to help check in rcu context > nfsd: no need get cache ref when protected by rcu > SUNRPC: no need get cache ref when protected by rcu > nfsd: fix UAF when access ex_uuid or ex_stats > > fs/nfsd/export.c | 25 ++++++++++------- > include/linux/sunrpc/cache.h | 2 ++ > net/sunrpc/cache.c | 53 ++++++++++++++++++++---------------- > 3 files changed, 46 insertions(+), 34 deletions(-) > A much nicer fix. My only minor nit is that it might be nice to add a kerneldoc header over cache_check_rcu, so that it's clear that it doesn't touch the cache_head refcount like cache_check does. Either way, you can add: Reviewed-by: Jeff Layton <jlayton@kernel.org>
After f8c989a0c89a ("nfsd: release svc_expkey/svc_export with rcu_work"), svc_export_put/expkey_put will call path_put with async mode. This can lead some unexpected failure: mkdir /mnt/sda mkfs.xfs -f /dev/sda echo "/ *(rw,no_root_squash,fsid=0)" > /etc/exports echo "/mnt *(rw,no_root_squash,fsid=1)" >> /etc/exports exportfs -ra service nfs-server start mount -t nfs -o vers=4.0 127.0.0.1:/mnt /mnt1 mount /dev/sda /mnt/sda touch /mnt1/sda/file exportfs -r umount /mnt/sda # failed unexcepted The touch above will finally call nfsd_cross_mnt, add refcount to mount, and then add cache_head. Before this commit, exportfs -r will call cache_flush to cleanup all cache_head, and path_put in svc_export_put/expkey_put will be finished with sync mode. So, the latter umount will always success. However, after this commit, path_put will be called with async mode, the latter umount may failed, and if we add some delay, umount will success too. Personally I think this bug and should be fixed. We first revert before bugfix patch, and then fix the original bug with a different way. v1->v2: 1. do not change cache_check, instead add cache_check_rcu and use it in c_show/e_show 2. the first patch has been applied Yang Erkun (4): SUNRPC: introduce cache_check_rcu to help check in rcu context nfsd: no need get cache ref when protected by rcu SUNRPC: no need get cache ref when protected by rcu nfsd: fix UAF when access ex_uuid or ex_stats fs/nfsd/export.c | 25 ++++++++++------- include/linux/sunrpc/cache.h | 2 ++ net/sunrpc/cache.c | 53 ++++++++++++++++++++---------------- 3 files changed, 46 insertions(+), 34 deletions(-)