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]
Date:   Fri, 26 Feb 2021 16:34:53 +0530
From:   Pratyush Yadav <p.yadav@...com>
To:     Arnd Bergmann <arnd@...nel.org>
CC:     Mark Brown <broonie@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Emil Renner Berthing <kernel@...il.dk>,
        Arnd Bergmann <arnd@...db.de>,
        Jon Lin <jon.lin@...k-chips.com>,
        Chris Ruehl <chris.ruehl@...ys.com.hk>,
        Alexander Kochetkov <al.kochet@...il.com>,
        Johan Jonker <jbx6244@...il.com>,
        Vincent Pelletier <plr.vincent@...il.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC support" 
        <linux-rockchip@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] spi: rockchip: avoid objtool warning

On 26/02/21 10:49AM, Arnd Bergmann wrote:
> On Fri, Feb 26, 2021 at 9:16 AM 'Pratyush Yadav' via Clang Built Linux
> <clang-built-linux@...glegroups.com> wrote:
> >
> > Hi,
> >
> > On 25/02/21 01:55PM, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@...db.de>
> > >
> > > Building this file with clang leads to a an unreachable code path
> > > causing a warning from objtool:
> > >
> > > drivers/spi/spi-rockchip.o: warning: objtool: rockchip_spi_transfer_one()+0x2e0: sibling call from callable instruction with modified stack frame
> > >
> > > Use BUG() instead of unreachable() to avoid the undefined behavior
> > > if it does happen.
> > >
> > > Fixes: 65498c6ae241 ("spi: rockchip: support 4bit words")
> > > Signed-off-by: Arnd Bergmann <arnd@...db.de>
> > > ---
> > >  drivers/spi/spi-rockchip.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> > > index 936ef54e0903..972beac1169a 100644
> > > --- a/drivers/spi/spi-rockchip.c
> > > +++ b/drivers/spi/spi-rockchip.c
> > > @@ -521,7 +521,7 @@ static void rockchip_spi_config(struct rockchip_spi *rs,
> > >                * ctlr->bits_per_word_mask, so this shouldn't
> > >                * happen
> > >                */
> > > -             unreachable();
> > > +             BUG();
> >
> > checkpatch says:
> >
> >   Avoid crashing the kernel - try using WARN_ON & recovery code rather
> >   than BUG() or BUG_ON()
> >
> > Which makes sense to me. This is not something bad enough to justify
> > crashing the kernel.
> 
> I thought about rewriting it more thoroughly when I wrote the patch, but
> couldn't come up with a good alternative, so I did the simplest change
> in this direction, replacing the silent crash with a loud one.
> 
> Should we just dev_warn() and return instead, hoping that it won't do
> more harm?

Hmm... I'm not very familiar with this device or driver so take what I 
say with a grain of salt. On the surface level it looks like it might 
end up doing something wrong or unexpected.

Returning an error code from this function (along with the dev_warn() or 
WARN_ON()) is the most sensible thing to do IMO. If the SPI layer sends 
an invalid value then the driver should be well within its rights to 
refuse the transaction. The function is currently void but making it 
return int seems fairly straightforward.

> 
> The backtrace from WARN_ON() probably doesn't help here.
> 
>        Arnd

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ