netdev - Re: [PATCH net] net/smc: delete buf_desc from buffer list under lock protection

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: <dedb6046-83a6-4bda-bf1d-ae77a8cda972@linux.alibaba.com>
Date: Fri, 2 Aug 2024 09:55:06 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: shaozhengchao <shaozhengchao@...wei.com>, wenjia@...ux.ibm.com,
 jaka@...ux.ibm.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com
Cc: alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
 linux-s390@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net/smc: delete buf_desc from buffer list under lock
 protection



On 2024/7/31 18:32, shaozhengchao wrote:
> Hi Wen Gu:
>    "The operations to link group buffer list should be protected by
> sndbufs_lock or rmbs_lock" It seems that the logic is smooth. But will
> this really happen? Because no process is in use with the link group,
> does this mean that there is no concurrent scenario?
> 

Hi Zhengchao,

Yes, I am also very conflicted about whether to add lock protection.
 From the code, it appears that when __smc_lgr_free_bufs is called, the
link group has already been removed from the lgr_list, so theoretically
there should be no contention (e.g. add to buf_list). However, in order
to maintain consistency with other lgr buf_list operations and to guard
against unforeseen or future changes, I have added lock protection here
as well.

Thanks!

> Thank you
> 
> Zhengchao Shao
> 
> On 2024/7/31 17:31, Wen Gu wrote:
>> The operations to link group buffer list should be protected by
>> sndbufs_lock or rmbs_lock. So fix it.
>>
>> Fixes: 3e034725c0d8 ("net/smc: common functions for RMBs and send buffers")
>> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
>> ---
>>   net/smc/smc_core.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 3b95828d9976..ecfea8c38da9 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1368,18 +1368,24 @@ static void __smc_lgr_free_bufs(struct smc_link_group *lgr, bool is_rmb)
>>   {
>>       struct smc_buf_desc *buf_desc, *bf_desc;
>>       struct list_head *buf_list;
>> +    struct rw_semaphore *lock;
>>       int i;
>>       for (i = 0; i < SMC_RMBE_SIZES; i++) {
>> -        if (is_rmb)
>> +        if (is_rmb) {
>>               buf_list = &lgr->rmbs[i];
>> -        else
>> +            lock = &lgr->rmbs_lock;
>> +        } else {
>>               buf_list = &lgr->sndbufs[i];
>> +            lock = &lgr->sndbufs_lock;
>> +        }
>> +        down_write(lock);
>>           list_for_each_entry_safe(buf_desc, bf_desc, buf_list,
>>                        list) {
>>               list_del(&buf_desc->list);
>>               smc_buf_free(lgr, is_rmb, buf_desc);
>>           }
>> +        up_write(lock);
>>       }
>>   }