Re: [PATCH 5/5] cachefiles: add missing lock protection when polling

[Baokun Li <libaokun@huaweicloud.com>]

Hi Xiang,

On 2024/4/24 12:29, Gao Xiang wrote:
> Hi Baokun,
>
> On 2024/4/24 11:34, libaokun@huaweicloud.com wrote:
>> From: Jingbo Xu <jefflexu@linux.alibaba.com>
>>
>> Add missing lock protection in poll routine when iterating xarray,
>> otherwise:
>>
>> Even with RCU read lock held, only the slot of the radix tree is
>> ensured to be pinned there, while the data structure (e.g. struct
>> cachefiles_req) stored in the slot has no such guarantee.  The poll
>> routine will iterate the radix tree and dereference cachefiles_req
>> accordingly.  Thus RCU read lock is not adequate in this case and
>> spinlock is needed here.
>>
>> Fixes: b817e22b2e91 ("cachefiles: narrow the scope of triggering 
>> EPOLLIN events in ondemand mode")
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
>
> I'm not sure why this patch didn't send upstream,
> https://gitee.com/anolis/cloud-kernel/commit/324ecaaa10fefb0e3d94b547e3170e40b90cda1f 
>
>
Yes, this issue blocks our tests, so this commit is adapted to upstream 
here.

> But since we're now working on upstreaming, so let's drop
> the previous in-house review tags..
>
> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
>
> Thanks,
> Gao Xiang

Ok, thanks for the review!

Cheers,
Baokun
>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   fs/cachefiles/daemon.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
>> index 6465e2574230..73ed2323282a 100644
>> --- a/fs/cachefiles/daemon.c
>> +++ b/fs/cachefiles/daemon.c
>> @@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct 
>> file *file,
>>         if (cachefiles_in_ondemand_mode(cache)) {
>>           if (!xa_empty(&cache->reqs)) {
>> -            rcu_read_lock();
>> +            xas_lock(&xas);
>>               xas_for_each_marked(&xas, req, ULONG_MAX, 
>> CACHEFILES_REQ_NEW) {
>>                   if (!cachefiles_ondemand_is_reopening_read(req)) {
>>                       mask |= EPOLLIN;
>>                       break;
>>                   }
>>               }
>> -            rcu_read_unlock();
>> +            xas_unlock(&xas);
>>           }
>>       } else {
>>           if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))