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  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:   Mon, 21 Dec 2020 18:01:41 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Miguel Ojeda <ojeda@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH] net: lantiq_etop: check the result of request_irq()

On Tue, Dec 22, 2020 at 12:59:08AM +0900, Masahiro Yamada wrote:
> On Tue, Dec 22, 2020 at 12:26 AM Andrew Lunn <andrew@...n.ch> wrote:
> >
> > On Mon, Dec 21, 2020 at 02:43:23PM +0900, Masahiro Yamada wrote:
> > > The declaration of request_irq() in <linux/interrupt.h> is marked as
> > > __must_check.
> > >
> > > Without the return value check, I see the following warnings:
> > >
> > > drivers/net/ethernet/lantiq_etop.c: In function 'ltq_etop_hw_init':
> > > drivers/net/ethernet/lantiq_etop.c:273:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   273 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/net/ethernet/lantiq_etop.c:281:4: warning: ignoring return value of 'request_irq', declared with attribute warn_unused_result [-Wunused-result]
> > >   281 |    request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
> > >       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Reported-by: Miguel Ojeda <ojeda@...nel.org>
> > > Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> > > ---
> > >
> > >  drivers/net/ethernet/lantiq_etop.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> > > index 2d0c52f7106b..960494f9752b 100644
> > > --- a/drivers/net/ethernet/lantiq_etop.c
> > > +++ b/drivers/net/ethernet/lantiq_etop.c
> > > @@ -264,13 +264,18 @@ ltq_etop_hw_init(struct net_device *dev)
> > >       for (i = 0; i < MAX_DMA_CHAN; i++) {
> > >               int irq = LTQ_DMA_CH0_INT + i;
> > >               struct ltq_etop_chan *ch = &priv->ch[i];
> > > +             int ret;
> > >
> > >               ch->idx = ch->dma.nr = i;
> > >               ch->dma.dev = &priv->pdev->dev;
> > >
> > >               if (IS_TX(i)) {
> > >                       ltq_dma_alloc_tx(&ch->dma);
> > > -                     request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     ret = request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> > > +                     if (ret) {
> > > +                             netdev_err(dev, "failed to request irq\n");
> > > +                             return ret;
> >
> > You need to cleanup what ltq_dma_alloc_tx() did.
> 
> 
> Any failure from this function will roll back
> in the following paths:
> 
>   ltq_etop_hw_exit()
>      -> ltq_etop_free_channel()
>           -> ltq_dma_free()
> 
> 
> So, dma is freed anyway.

O.K, thanks for the information. 

> One problem I see is,
> ltq_etop_hw_exit() frees all DMA channels,
> some of which may not have been allocated yet.
> 
> If it is a bug, it is an existing bug.
> 
> 
> >
> > > +                     }
> > >               } else if (IS_RX(i)) {
> > >                       ltq_dma_alloc_rx(&ch->dma);
> > >                       for (ch->dma.desc = 0; ch->dma.desc < LTQ_DESC_NUM;
> > > @@ -278,7 +283,11 @@ ltq_etop_hw_init(struct net_device *dev)
> > >                               if (ltq_etop_alloc_skb(ch))
> > >                                       return -ENOMEM;
> 
> 
> This -ENOMEM does not roll back anything here.
> 
> As stated above, dma_free_coherent() is called.
> The problem is, ltq_etop_hw_exit() rolls back too much.
> 
> If your requirement is "this driver is completely wrong. Please rewrite it",
> sorry, I cannot (unless I am paid to do so).
> 
> I am just following this driver's roll-back model.
> 
> Please do not expect more to a person who
> volunteers to eliminate build warnings.

There is a balance here. We should not remove a warning unless we
properly fix the warning. Otherwise having the warning is pointless.

So please leave the warning in place, and maybe somebody else will
fully fix it.

      Andrew

Powered by blists - more mailing lists