netdev - Re: [RFC PATCH 0/5] nfsd/sunrpc: cleanup resource with sync mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <173508083549.11072.301112272697956815@noble.neil.brown.name>
Date: Wed, 25 Dec 2024 09:53:55 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Yang Erkun" <yangerkun@...weicloud.com>
Cc: chuck.lever@...cle.com, jlayton@...nel.org, okorniev@...hat.com,
 Dai.Ngo@...cle.com, tom@...pey.com, trondmy@...nel.org, anna@...nel.org,
 linux-nfs@...r.kernel.org, netdev@...r.kernel.org, yangerkun@...wei.com,
 yangerkun@...weicloud.com, yi.zhang@...wei.com
Subject: Re: [RFC PATCH 0/5] nfsd/sunrpc: cleanup resource with sync mode

On Tue, 17 Dec 2024, Yang Erkun wrote:
> From: Yang Erkun <yangerkun@...wei.com>
> 
> 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.

Thanks for these patches.  I think they are certainly a better approach
to fixing the problem - well done.

My only thought was that instead of changing how cache_check() works, we
could introduce cache_check_rcu() which doesn't drop the ref.
cache_check() would then just call that then optionally drop the ref.
I'm not convinced that is better, so I'm just mentioning it in case
anyone else wants to agree.  I'm happy for the patch set to be applied
as-is.

 Reviewed-By: NeilBrown <neilb@...e.de>

Thanks,
NeilBrown