linux-kernel - Re: [PATCH net 7/9] rxrpc: Fix I/O thread stop

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

Date:   Mon, 19 Dec 2022 00:20:05 +0000
From:   David Howells <dhowells@...hat.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     dhowells@...hat.com, netdev@...r.kernel.org,
        Marc Dionne <marc.dionne@...istor.com>,
        linux-afs@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 7/9] rxrpc: Fix I/O thread stop

Hillf Danton <hdanton@...a.com> wrote:

> > So once we've observed that we've been asked to stop, we need to check if
> > there's more work to be done and, if so, do that work first.
> 
> In line with
> 
> 	if (condition)
> 		return;
> 	add to wait queue
> 	if (!condition)
> 		schedule();
> 
> this change should look like
> 
>    		if (!skb_queue_empty(&local->rx_queue) ...)
>  			continue;
> 
>  		if (kthread_should_stop())
>    			if (!skb_queue_empty(&local->rx_queue) ...)
>  				continue;
> 			else
>   				break;
> 
> as checking condition once barely makes sense.

Note that these are not really analogous.  The add-to-wait-queue step is
significantly more expensive than kthread_should_stop() and requires removal
in the event that the condition becomes true in the window.

In the case of kthread_should_stop(), it's just a test_bit() of a word that's
in a cacheline not going to get changed until the thread is stopped.  Testing
the value first and then checking the condition should be fine as the stop
flag can be shared in the cpu's data cache until it's set.

Also from a code-maintenance PoV, I don't want to write the condition twice if
I can avoid it.  That allows for the two copies to get out of sync.

> Because of a bit complex condition does not mean checking it once is neither
> sane nor correct.

So you agree with me, I think?

David