netdev - Re: [RFC PATCH 1/5] nfsd: Revert "nfsd: release svc_expkey/svc_export with rcu

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: <4cfae0e3-c5dc-4726-bbc8-dc0b5395d8cd@oracle.com>
Date: Mon, 16 Dec 2024 09:52:33 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Yang Erkun <yangerkun@...weicloud.com>, jlayton@...nel.org, neilb@...e.de,
        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
Cc: yangerkun@...wei.com, yi.zhang@...wei.com
Subject: Re: [RFC PATCH 1/5] nfsd: Revert "nfsd: release svc_expkey/svc_export
 with rcu_work"

On 12/16/24 9:21 AM, Yang Erkun wrote:
> From: Yang Erkun <yangerkun@...wei.com>
> 
> This reverts commit f8c989a0c89a75d30f899a7cabdc14d72522bb8d.

The reverted commit is in v6.13-rc1, looks like. Should I apply 1/5 to
v6.13-rc ?

The remaining patches in this series will need review and testing,
so I would like to reserve them for a later merge window, if that
is OK with you.


> Before this commit, svc_export_put or expkey_put will call path_put with
> sync mode. After this commit, path_put will be called with async mode.
> And this can lead the unexpected results show as follow.
> 
> 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 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.
> 
> Fixes: f8c989a0c89a ("nfsd: release svc_expkey/svc_export with rcu_work")
> Signed-off-by: Yang Erkun <yangerkun@...wei.com>
> ---
>   fs/nfsd/export.c | 31 ++++++-------------------------
>   fs/nfsd/export.h |  4 ++--
>   2 files changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index eacafe46e3b6..aa4712362b3b 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -40,24 +40,15 @@
>   #define	EXPKEY_HASHMAX		(1 << EXPKEY_HASHBITS)
>   #define	EXPKEY_HASHMASK		(EXPKEY_HASHMAX -1)
>   
> -static void expkey_put_work(struct work_struct *work)
> +static void expkey_put(struct kref *ref)
>   {
> -	struct svc_expkey *key =
> -		container_of(to_rcu_work(work), struct svc_expkey, ek_rcu_work);
> +	struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref);
>   
>   	if (test_bit(CACHE_VALID, &key->h.flags) &&
>   	    !test_bit(CACHE_NEGATIVE, &key->h.flags))
>   		path_put(&key->ek_path);
>   	auth_domain_put(key->ek_client);
> -	kfree(key);
> -}
> -
> -static void expkey_put(struct kref *ref)
> -{
> -	struct svc_expkey *key = container_of(ref, struct svc_expkey, h.ref);
> -
> -	INIT_RCU_WORK(&key->ek_rcu_work, expkey_put_work);
> -	queue_rcu_work(system_wq, &key->ek_rcu_work);
> +	kfree_rcu(key, ek_rcu);
>   }
>   
>   static int expkey_upcall(struct cache_detail *cd, struct cache_head *h)
> @@ -364,26 +355,16 @@ static void export_stats_destroy(struct export_stats *stats)
>   					    EXP_STATS_COUNTERS_NUM);
>   }
>   
> -static void svc_export_put_work(struct work_struct *work)
> +static void svc_export_put(struct kref *ref)
>   {
> -	struct svc_export *exp =
> -		container_of(to_rcu_work(work), struct svc_export, ex_rcu_work);
> -
> +	struct svc_export *exp = container_of(ref, struct svc_export, h.ref);
>   	path_put(&exp->ex_path);
>   	auth_domain_put(exp->ex_client);
>   	nfsd4_fslocs_free(&exp->ex_fslocs);
>   	export_stats_destroy(exp->ex_stats);
>   	kfree(exp->ex_stats);
>   	kfree(exp->ex_uuid);
> -	kfree(exp);
> -}
> -
> -static void svc_export_put(struct kref *ref)
> -{
> -	struct svc_export *exp = container_of(ref, struct svc_export, h.ref);
> -
> -	INIT_RCU_WORK(&exp->ex_rcu_work, svc_export_put_work);
> -	queue_rcu_work(system_wq, &exp->ex_rcu_work);
> +	kfree_rcu(exp, ex_rcu);
>   }
>   
>   static int svc_export_upcall(struct cache_detail *cd, struct cache_head *h)
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index 6f2fbaae01fa..4d92b99c1ffd 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -75,7 +75,7 @@ struct svc_export {
>   	u32			ex_layout_types;
>   	struct nfsd4_deviceid_map *ex_devid_map;
>   	struct cache_detail	*cd;
> -	struct rcu_work		ex_rcu_work;
> +	struct rcu_head		ex_rcu;
>   	unsigned long		ex_xprtsec_modes;
>   	struct export_stats	*ex_stats;
>   };
> @@ -92,7 +92,7 @@ struct svc_expkey {
>   	u32			ek_fsid[6];
>   
>   	struct path		ek_path;
> -	struct rcu_work		ek_rcu_work;
> +	struct rcu_head		ek_rcu;
>   };
>   
>   #define EX_ISSYNC(exp)		(!((exp)->ex_flags & NFSEXP_ASYNC))


-- 
Chuck Lever