linux-kernel - Re: [PATCH] i2c: brcmstb: Add support for atomic transfers

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: <ba3c0972-2ba2-4df7-826e-a7634027b967@gmail.com>
Date:   Wed, 11 Oct 2023 12:23:31 +0200
From:   Gregor Riepl <onitake@...il.com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Cc:     Kamal Dasu <kamal.dasu@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Andi Shyti <andi.shyti@...nel.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Wolfram Sang <wsa@...nel.org>
Subject: Re: [PATCH] i2c: brcmstb: Add support for atomic transfers

Hi,

I admit that I don't understand the I²C subsystem very well, but doesn't 
this introduce a potential race condition?

 > ...
 > @@ -240,7 +241,7 @@ static int brcmstb_i2c_wait_for_completion(struct 
brcmstb_i2c_dev *dev)
 > ...
> -	if (dev->irq >= 0) {
> +	if (dev->irq >= 0 && !dev->atomic) {
 > ...
 > @@ -287,7 +288,7 @@ static int brcmstb_send_i2c_cmd(struct 
brcmstb_i2c_dev *dev,
 > ...
> -	if (dev->irq >= 0)
> +	if (dev->irq >= 0 && !dev->atomic)
 > ...
 > +static int brcmstb_i2c_xfer_atomic(struct i2c_adapter *adapter,
 > +				   struct i2c_msg msgs[], int num)
 > ...
> +	dev->atomic = true;
> +	ret = brcmstb_i2c_xfer(adapter, msgs, num);
> +	dev->atomic = false;
> ...

What happens when one of the if() branches is taken in one thread while 
another thread is just executing the assignment of the atomic flag? My 
expectation would be that the first tread still sees the old flag value 
and happily executes the branch, while brcmstb_i2c_xfer_atomic() sets 
the flag just after and initiates a transfer.

I'd expect that access to the flag must be atomic as well, so maybe 
something like 
https://www.kernel.org/doc/html/latest/core-api/wrappers/atomic_t.html 
is needed, or some other synchronization mechanism.

Or is it guaranteed that brcmstb_i2c_wait_for_completion() and 
brcmstb_send_i2c_cmd() can only be called from the same thread as 
brcmstb_i2c_xfer_atomic() ?

Regards,
Gregor