netdev - Re: [PATCH 1/6] net/rds: Avoid queuing superfluous send and recv work

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: <3b02c34d2a15b4529b384ab91b27e5be0f941130.camel@oracle.com>
Date: Wed, 12 Mar 2025 07:50:11 +0000
From: Allison Henderson <allison.henderson@...cle.com>
To: "kuba@...nel.org" <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/6] net/rds: Avoid queuing superfluous send and recv work

On Fri, 2025-03-07 at 18:53 -0800, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 20:28:57 +0000 Allison Henderson wrote:
> > > Let's be precise, can you give an example of 2 execution threads
> > > and memory accesses which have to be ordered.  
> > 
> > Hi Jakub,
> > 
> > I just realized my last response referred to bits and functions in the next patch instead this of one.  Apologies for
> > the confusion!  For this thread example though, I think a pair of threads in rds_send_worker and rds_sendmsg would be a
> > good example?  How about this:
> > 
> > Thread A:
> >   Calls rds_send_worker()
> >     calls rds_clear_queued_send_work_bit()
> >       clears RDS_SEND_WORK_QUEUED in cp->cp_flags
> >     calls rds_send_xmit()
> >     calls cond_resched()
> > 
> > Thread B:
> >    Calls rds_sendmsg()
> >    Calls rds_send_xmit
> >    Calls rds_cond_queue_send_work 
> >       checks and sets RDS_SEND_WORK_QUEUED in cp->cp_flags
> 
> We need at least two memory locations if we want to talk about ordering.
> In your example we have cp_flags, but the rest is code.
> What's the second memory location.
> Take a look at e592b5110b3e9393 for an example of a good side by side
> thread execution.. listing(?):
> 
>     Thread1 (oa_tc6_start_xmit)     Thread2 (oa_tc6_spi_thread_handler)
>     ---------------------------     -----------------------------------
>     - if waiting_tx_skb is NULL
>                                     - if ongoing_tx_skb is NULL
>                                     - ongoing_tx_skb = waiting_tx_skb
>     - waiting_tx_skb = skb
>                                     - waiting_tx_skb = NULL
>                                     ...
>                                     - ongoing_tx_skb = NULL
>     - if waiting_tx_skb is NULL
>     - waiting_tx_skb = skb
> 
> 
> This makes it pretty clear what fields are at play and how the race
> happens.
Hi Jakub,

I suppose the second address would have to be the queue itself wouldn't it?  We have a flag that's meant to avoid
threads racing to access a queue, so it would make sense that the addresses of interest would be the flag and the queue.
Which is cp->cp_send_w in the send example.  So if we adjusted our example to include the queue access, then it would
look like this: 

Thread A:					Thread B:
-----------------------------------		-----------------------------------
						Calls rds_sendmsg()
						   Calls rds_send_xmit()
						      Calls rds_cond_queue_send_work()   
Calls rds_send_worker()					
  calls rds_clear_queued_send_work_bit()		   
    clears RDS_SEND_WORK_QUEUED in cp->cp_flags		
      						         checks RDS_SEND_WORK_QUEUED in cp->cp_flags
      						         but sees stale value
      						         Skips queuing on cp->cp_send_w when it should not
    Calls rds_send_xmit()
       Calls rds_cond_queue_send_work()
          queues work on cp->cp_send_w

And then if we have the barriers, then the example would look like this:

Thread A:					Thread B:
-----------------------------------		-----------------------------------
						Calls rds_sendmsg()
						   Calls rds_send_xmit()
						      Calls rds_cond_queue_send_work()   
Calls rds_send_worker()					
  calls rds_clear_queued_send_work_bit()		   
    clears RDS_SEND_WORK_QUEUED in cp->cp_flags		
      						         checks RDS_SEND_WORK_QUEUED in cp->cp_flags
      						         Queues work on on cp->cp_send_w
    Calls rds_send_xmit()
       Calls rds_cond_queue_send_work()
          skips queueing work on cp->cp_send_w

I think the barriers also make sure thread A's call to rds_send_xmit() happens after the clear_bit() too.  Otherwise it
may be possible that it is reordered, and then we get another missed work item there too.  I hope this helps some?  Let
me know if that makes sense or if you think there's a better way it could be managed.  Thank you!

Allison