netdev - Re: [PATCH 3/4] ll_temac: free everything on error path

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: <AANLkTilMaITOMsn_9MmZ7-zHwpvTd2ZYjj4UUWOloYsn@mail.gmail.com>
Date:	Thu, 8 Jul 2010 17:56:09 +0400
From:	Denis Kirjanov <kirjanov@...il.com>
To:	Kulikov Vasiliy <segooon@...il.com>
Cc:	Kernel Janitors <kernel-janitors@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	John Linn <john.linn@...inx.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Jiri Pirko <jpirko@...hat.com>,
	Brian Hill <brian.hill@...inx.com>, netdev@...r.kernel.org
Subject: Re: [PATCH 3/4] ll_temac: free everything on error path

On Thu, Jul 8, 2010 at 5:46 PM, Kulikov Vasiliy <segooon@...il.com> wrote:
> On Thu, Jul 08, 2010 at 17:16 +0400, Denis Kirjanov wrote:
>> On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@...il.com> wrote:
>> > temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
>> >
>> > Signed-off-by: Kulikov Vasiliy <segooon@...il.com>
>> > ---
>> >  drivers/net/ll_temac_main.c |   30 +++++++++++++++++++++++++-----
>> >  1 files changed, 25 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
>> > index a2da3d7..38d658a 100644
>> > --- a/drivers/net/ll_temac_main.c
>> > +++ b/drivers/net/ll_temac_main.c
>> > @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >        struct temac_local *lp = netdev_priv(ndev);
>> >        struct sk_buff *skb;
>> >        int i;
>> > +       int tx_bd_v_size, rx_bd_v_size;
>> >
>> >        lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
>> >        if (!lp->rx_skb) {
>> > @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >        }
>> >        /* allocate the tx and rx ring buffer descriptors. */
>> >        /* returns a virtual addres and a physical address. */
>> > +       tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
>> >        lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
>           ^^^^^^^^^^^
>            1st
>> > -                                        sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>> > +                                       tx_bd_v_size,
>> >                                         &lp->tx_bd_p, GFP_KERNEL);
>> >        if (!lp->tx_bd_v) {
>> >                dev_err(&ndev->dev,
>> >                                "unable to allocate DMA TX buffer descriptors");
>> > -               goto out;
>> > +               goto err_rx_skb;
>> >        }
>> > +       rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
>> >        lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
>           ^^^^^^^^^^^
>            2nd
>> > -                                        sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>> > +                                        rx_bd_v_size,
>> >                                         &lp->rx_bd_p, GFP_KERNEL);
>> >        if (!lp->rx_bd_v) {
>> >                dev_err(&ndev->dev,
>> >                                "unable to allocate DMA RX buffer descriptors");
>> > -               goto out;
>> > +               goto err_tx_bd;
>> >        }
>> >
>> >        memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
>> > @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> >                if (skb == 0) {
>> >                        dev_err(&ndev->dev, "alloc_skb error %d\n", i);
>> > -                       goto out;
>> > +                       goto err_dma_single;
>> >                }
>> >                lp->rx_skb[i] = skb;
>> >                /* returns physical address of skb->data */
>> > @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> >        return 0;
>> >
>> > +err_dma_single:
>> > +       for (i = 0; i < RX_BD_NUM; i++) {
>> > +               if (lp->rx_skb[i] == NULL)
>> > +                       break;
>> > +
>> > +               kfree_skb(lp->rx_skb[i]);
>> > +               dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>> > +                               XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>> > +       }
>> > +
>> > +       dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
>> > +                               lp->rx_bd_v, lp->rx_bd_p);
>                                    ^^^^^^^^^^^
>                                     2nd
>> > +err_tx_bd:
>> > +       dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
>> > +                               lp->tx_bd_v, lp->tx_bd_p);
>                                    ^^^^^^^^^^^
>                                     1st
>> > +err_rx_skb:
>> > +       kfree(lp->rx_skb);
>> >  out:
>> >        return -ENOMEM;
>> >  }
>> > --
>> > 1.7.0.4
>> This is not enough. DMA resources also must be released on exit.
> Could you point to it? I see only two variables with allocated DMA (see above).
> --
I just sent a patch that fixes this.

> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Regards,
Denis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html