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: <e457414e-f106-3c69-a27f-056101c9cbd9@huaweicloud.com>
Date: Tue, 17 Dec 2024 09:05:08 +0800
From: yangerkun <yangerkun@...weicloud.com>
To: Chuck Lever <chuck.lever@...cle.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"



在 2024/12/16 22:52, Chuck Lever 写道:
> 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.

Thanks, it's ok to me!

> 
> 
>> 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))
> 
>