linux-kernel - Re: [PATCH net 2/6] net/smc: set SOCK_NOSPACE when send_remaining but no sndbuf

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] [day] [month] [year] [list]

Message-ID: <2b9d0b12-6830-48d5-ad65-49f401c4e365@linux.alibaba.com>
Date: Wed, 4 Dec 2024 15:12:42 +0800
From: Guangguan Wang <guangguan.wang@...ux.alibaba.com>
To: Paolo Abeni <pabeni@...hat.com>, wenjia@...ux.ibm.com,
 jaka@...ux.ibm.com, alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
 guwen@...ux.alibaba.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, horms@...nel.org
Cc: linux-rdma@...r.kernel.org, linux-s390@...r.kernel.org,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/6] net/smc: set SOCK_NOSPACE when send_remaining but
 no sndbuf_space left



On 2024/12/3 18:04, Paolo Abeni wrote:
> 
> 
> On 11/28/24 13:14, Guangguan Wang wrote:
>> When application sending data more than sndbuf_space, there have chances
>> application will sleep in epoll_wait, and will never be wakeup again. This
>> is caused by a race between smc_poll and smc_cdc_tx_handler.
>>
>> application                                      tasklet
>> smc_tx_sendmsg(len > sndbuf_space)   |
>> epoll_wait for EPOLL_OUT,timeout=0   |
>>   smc_poll                           |
>>     if (!smc->conn.sndbuf_space)     |
>>                                      |  smc_cdc_tx_handler
>>                                      |    atomic_add sndbuf_space
>>                                      |    smc_tx_sndbuf_nonfull
>>                                      |      if (!test_bit SOCK_NOSPACE)
>>                                      |        do not sk_write_space;
>>       set_bit SOCK_NOSPACE;          |
>>     return mask=0;                   |
>>
>> Application will sleep in epoll_wait as smc_poll returns 0. And
>> smc_cdc_tx_handler will not call sk_write_space because the SOCK_NOSPACE
>> has not be set. If there is no inflight cdc msg, sk_write_space will not be
>> called any more, and application will sleep in epoll_wait forever.
>> So set SOCK_NOSPACE when send_remaining but no sndbuf_space left in
>> smc_tx_sendmsg, to ensure call sk_write_space in smc_cdc_tx_handler
>> even when the above race happens.
> 
> I think it should be preferable to address the mentioned race the same
> way as tcp_poll(). i.e. checking again smc->conn.sndbuf_space after
> setting the NOSPACE bit with appropriate barrier, see:
> 
> https://elixir.bootlin.com/linux/v6.12.1/source/net/ipv4/tcp.c#L590
> 
> that will avoid additional, possibly unneeded atomic operation in the tx
> path (the application could do the next sendmsg()/poll() call after that
> the send buf has been freed) and will avoid some code duplication.
> 
> Cheers,
> 
> Paolo

Hi, Paolo

Thanks for advice, and the way in tcp_poll() seems a better solution for this race.
I will retest it, and resend a new version of patch if it works.

Thanks,
Guangguan Wang