| Message ID | 20200110115415.75683-4-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: axienet: Error handling, SGMII and 64-bit DMA fixes | expand |
> -----Original Message----- > From: Andre Przywara <andre.przywara@arm.com> > Sent: Friday, January 10, 2020 5:24 PM > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey > <radheys@xilinx.com> > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > When axienet_dma_bd_init() bails out during the initialisation process, > it might do so with parts of the structure already allocated and > initialised, while other parts have not been touched yet. Before > returning in this case, we call axienet_dma_bd_release(), which does not > take care of this corner case. > This is most obvious by the first loop happily dereferencing > lp->rx_bd_v, which we actually check to be non NULL *afterwards*. > > Make sure we only unmap or free already allocated structures, by: > - directly returning with -ENOMEM if nothing has been allocated at all > - checking for lp->rx_bd_v to be non-NULL *before* using it > - only unmapping allocated DMA RX regions > > This avoids NULL pointer dereferences when initialisation fails. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index 97482cf093ce..7e90044cf2d9 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct > net_device *ndev) > int i; > struct axienet_local *lp = netdev_priv(ndev); > > + /* If we end up here, tx_bd_v must have been DMA allocated. */ > + dma_free_coherent(ndev->dev.parent, > + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > + lp->tx_bd_v, > + lp->tx_bd_p); > + > + if (!lp->rx_bd_v) > + return; > + > for (i = 0; i < lp->rx_bd_num; i++) { > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > - lp->max_frm_size, DMA_FROM_DEVICE); > + /* A NULL skb means this descriptor has not been initialised > + * at all. > + */ > + if (!lp->rx_bd_v[i].skb) > + break; > + > dev_kfree_skb(lp->rx_bd_v[i].skb); > - } > > - if (lp->rx_bd_v) { > - dma_free_coherent(ndev->dev.parent, > - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > - lp->rx_bd_v, > - lp->rx_bd_p); > - } > - if (lp->tx_bd_v) { > - dma_free_coherent(ndev->dev.parent, > - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > - lp->tx_bd_v, > - lp->tx_bd_p); > + /* For each descriptor, we programmed cntrl with the (non- > zero) > + * descriptor size, after it had been successfully allocated. > + * So a non-zero value in there means we need to unmap it. > + */ > + if (lp->rx_bd_v[i].cntrl) I think it should ok to unmap w/o any check? > + dma_unmap_single(ndev->dev.parent, lp- > >rx_bd_v[i].phys, > + lp->max_frm_size, > DMA_FROM_DEVICE); > } > + > + dma_free_coherent(ndev->dev.parent, > + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > + lp->rx_bd_v, > + lp->rx_bd_p); > } > > /** > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device > *ndev) > sizeof(*lp->tx_bd_v) * lp- > >tx_bd_num, > &lp->tx_bd_p, GFP_KERNEL); > if (!lp->tx_bd_v) > - goto out; > + return -ENOMEM; > > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, > sizeof(*lp->rx_bd_v) * lp- > >rx_bd_num, > -- > 2.17.1
On Fri, 10 Jan 2020 15:14:46 +0000 Radhey Shyam Pandey <radheys@xilinx.com> wrote: Hi Radhey, thanks for having a look! > > -----Original Message----- > > From: Andre Przywara <andre.przywara@arm.com> > > Sent: Friday, January 10, 2020 5:24 PM > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey > > <radheys@xilinx.com> > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > > > When axienet_dma_bd_init() bails out during the initialisation process, > > it might do so with parts of the structure already allocated and > > initialised, while other parts have not been touched yet. Before > > returning in this case, we call axienet_dma_bd_release(), which does not > > take care of this corner case. > > This is most obvious by the first loop happily dereferencing > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*. > > > > Make sure we only unmap or free already allocated structures, by: > > - directly returning with -ENOMEM if nothing has been allocated at all > > - checking for lp->rx_bd_v to be non-NULL *before* using it > > - only unmapping allocated DMA RX regions > > > > This avoids NULL pointer dereferences when initialisation fails. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++------- > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > index 97482cf093ce..7e90044cf2d9 100644 > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct > > net_device *ndev) > > int i; > > struct axienet_local *lp = netdev_priv(ndev); > > > > + /* If we end up here, tx_bd_v must have been DMA allocated. */ > > + dma_free_coherent(ndev->dev.parent, > > + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > + lp->tx_bd_v, > > + lp->tx_bd_p); > > + > > + if (!lp->rx_bd_v) > > + return; > > + > > for (i = 0; i < lp->rx_bd_num; i++) { > > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > > - lp->max_frm_size, DMA_FROM_DEVICE); > > + /* A NULL skb means this descriptor has not been initialised > > + * at all. > > + */ > > + if (!lp->rx_bd_v[i].skb) > > + break; > > + > > dev_kfree_skb(lp->rx_bd_v[i].skb); > > - } > > > > - if (lp->rx_bd_v) { > > - dma_free_coherent(ndev->dev.parent, > > - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > - lp->rx_bd_v, > > - lp->rx_bd_p); > > - } > > - if (lp->tx_bd_v) { > > - dma_free_coherent(ndev->dev.parent, > > - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > - lp->tx_bd_v, > > - lp->tx_bd_p); > > + /* For each descriptor, we programmed cntrl with the (non- > > zero) > > + * descriptor size, after it had been successfully allocated. > > + * So a non-zero value in there means we need to unmap it. > > + */ > > > + if (lp->rx_bd_v[i].cntrl) > > I think it should ok to unmap w/o any check? Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a valid DMA address, so there is no special check for that, and unmapping DMA address 0 will probably go wrong at some point. So it's unlike kfree(NULL). Cheers, Andre. > > + dma_unmap_single(ndev->dev.parent, lp- > > >rx_bd_v[i].phys, > > + lp->max_frm_size, > > DMA_FROM_DEVICE); > > } > > + > > + dma_free_coherent(ndev->dev.parent, > > + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > + lp->rx_bd_v, > > + lp->rx_bd_p); > > } > > > > /** > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device > > *ndev) > > sizeof(*lp->tx_bd_v) * lp- > > >tx_bd_num, > > &lp->tx_bd_p, GFP_KERNEL); > > if (!lp->tx_bd_v) > > - goto out; > > + return -ENOMEM; > > > > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, > > sizeof(*lp->rx_bd_v) * lp- > > >rx_bd_num, > > -- > > 2.17.1 >
> -----Original Message----- > From: Andre Przywara <andre.przywara@arm.com> > Sent: Friday, January 10, 2020 9:13 PM > To: Radhey Shyam Pandey <radheys@xilinx.com> > Cc: David S . Miller <davem@davemloft.net>; Michal Simek > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>; > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > On Fri, 10 Jan 2020 15:14:46 +0000 > Radhey Shyam Pandey <radheys@xilinx.com> wrote: > > Hi Radhey, > > thanks for having a look! > > > > -----Original Message----- > > > From: Andre Przywara <andre.przywara@arm.com> > > > Sent: Friday, January 10, 2020 5:24 PM > > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey > > > <radheys@xilinx.com> > > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock > > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm- > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > > > > > When axienet_dma_bd_init() bails out during the initialisation process, > > > it might do so with parts of the structure already allocated and > > > initialised, while other parts have not been touched yet. Before > > > returning in this case, we call axienet_dma_bd_release(), which does not > > > take care of this corner case. > > > This is most obvious by the first loop happily dereferencing > > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*. > > > > > > Make sure we only unmap or free already allocated structures, by: > > > - directly returning with -ENOMEM if nothing has been allocated at all > > > - checking for lp->rx_bd_v to be non-NULL *before* using it > > > - only unmapping allocated DMA RX regions > > > > > > This avoids NULL pointer dereferences when initialisation fails. > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++------- > > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > index 97482cf093ce..7e90044cf2d9 100644 > > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct > > > net_device *ndev) > > > int i; > > > struct axienet_local *lp = netdev_priv(ndev); > > > > > > + /* If we end up here, tx_bd_v must have been DMA allocated. */ > > > + dma_free_coherent(ndev->dev.parent, > > > + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > + lp->tx_bd_v, > > > + lp->tx_bd_p); > > > + > > > + if (!lp->rx_bd_v) > > > + return; > > > + > > > for (i = 0; i < lp->rx_bd_num; i++) { > > > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > > > - lp->max_frm_size, DMA_FROM_DEVICE); > > > + /* A NULL skb means this descriptor has not been initialised > > > + * at all. > > > + */ > > > + if (!lp->rx_bd_v[i].skb) > > > + break; > > > + > > > dev_kfree_skb(lp->rx_bd_v[i].skb); > > > - } > > > > > > - if (lp->rx_bd_v) { > > > - dma_free_coherent(ndev->dev.parent, > > > - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > - lp->rx_bd_v, > > > - lp->rx_bd_p); > > > - } > > > - if (lp->tx_bd_v) { > > > - dma_free_coherent(ndev->dev.parent, > > > - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > - lp->tx_bd_v, > > > - lp->tx_bd_p); > > > + /* For each descriptor, we programmed cntrl with the (non- > > > zero) > > > + * descriptor size, after it had been successfully allocated. > > > + * So a non-zero value in there means we need to unmap it. > > > + */ > > > > > + if (lp->rx_bd_v[i].cntrl) > > > > I think it should ok to unmap w/o any check? > > Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a > valid DMA address, so there is no special check for that, and unmapping > DMA address 0 will probably go wrong at some point. So it's unlike > kfree(NULL). I mean if skb allocation is successful in _dma_bd_init then in release path we can assume .phys is always a valid address and skip rx_bd_v[i].cntrl check. > > Cheers, > Andre. > > > > > + dma_unmap_single(ndev->dev.parent, lp- > > > >rx_bd_v[i].phys, > > > + lp->max_frm_size, > > > DMA_FROM_DEVICE); > > > } > > > + > > > + dma_free_coherent(ndev->dev.parent, > > > + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > + lp->rx_bd_v, > > > + lp->rx_bd_p); > > > } > > > > > > /** > > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device > > > *ndev) > > > sizeof(*lp->tx_bd_v) * lp- > > > >tx_bd_num, > > > &lp->tx_bd_p, GFP_KERNEL); > > > if (!lp->tx_bd_v) > > > - goto out; > > > + return -ENOMEM; > > > > > > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, > > > sizeof(*lp->rx_bd_v) * lp- > > > >rx_bd_num, > > > -- > > > 2.17.1 > >
On Fri, 10 Jan 2020 17:05:45 +0000 Radhey Shyam Pandey <radheys@xilinx.com> wrote: Hi, > > -----Original Message----- > > From: Andre Przywara <andre.przywara@arm.com> > > Sent: Friday, January 10, 2020 9:13 PM > > To: Radhey Shyam Pandey <radheys@xilinx.com> > > Cc: David S . Miller <davem@davemloft.net>; Michal Simek > > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>; > > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > > > On Fri, 10 Jan 2020 15:14:46 +0000 > > Radhey Shyam Pandey <radheys@xilinx.com> wrote: > > > > Hi Radhey, > > > > thanks for having a look! > > > > > > -----Original Message----- > > > > From: Andre Przywara <andre.przywara@arm.com> > > > > Sent: Friday, January 10, 2020 5:24 PM > > > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey > > > > <radheys@xilinx.com> > > > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock > > > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm- > > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > > > > > > > When axienet_dma_bd_init() bails out during the initialisation process, > > > > it might do so with parts of the structure already allocated and > > > > initialised, while other parts have not been touched yet. Before > > > > returning in this case, we call axienet_dma_bd_release(), which does not > > > > take care of this corner case. > > > > This is most obvious by the first loop happily dereferencing > > > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*. > > > > > > > > Make sure we only unmap or free already allocated structures, by: > > > > - directly returning with -ENOMEM if nothing has been allocated at all > > > > - checking for lp->rx_bd_v to be non-NULL *before* using it > > > > - only unmapping allocated DMA RX regions > > > > > > > > This avoids NULL pointer dereferences when initialisation fails. > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++------- > > > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > index 97482cf093ce..7e90044cf2d9 100644 > > > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct > > > > net_device *ndev) > > > > int i; > > > > struct axienet_local *lp = netdev_priv(ndev); > > > > > > > > + /* If we end up here, tx_bd_v must have been DMA allocated. */ > > > > + dma_free_coherent(ndev->dev.parent, > > > > + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > > + lp->tx_bd_v, > > > > + lp->tx_bd_p); > > > > + > > > > + if (!lp->rx_bd_v) > > > > + return; > > > > + > > > > for (i = 0; i < lp->rx_bd_num; i++) { > > > > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > > > > - lp->max_frm_size, DMA_FROM_DEVICE); > > > > + /* A NULL skb means this descriptor has not been initialised > > > > + * at all. > > > > + */ > > > > + if (!lp->rx_bd_v[i].skb) > > > > + break; > > > > + > > > > dev_kfree_skb(lp->rx_bd_v[i].skb); > > > > - } > > > > > > > > - if (lp->rx_bd_v) { > > > > - dma_free_coherent(ndev->dev.parent, > > > > - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > > - lp->rx_bd_v, > > > > - lp->rx_bd_p); > > > > - } > > > > - if (lp->tx_bd_v) { > > > > - dma_free_coherent(ndev->dev.parent, > > > > - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > > - lp->tx_bd_v, > > > > - lp->tx_bd_p); > > > > + /* For each descriptor, we programmed cntrl with the (non- > > > > zero) > > > > + * descriptor size, after it had been successfully allocated. > > > > + * So a non-zero value in there means we need to unmap it. > > > > + */ > > > > > > > + if (lp->rx_bd_v[i].cntrl) > > > > > > I think it should ok to unmap w/o any check? > > > > Do you mean because .phys would be 0 if not initialised? AFAIK 0 can be a > > valid DMA address, so there is no special check for that, and unmapping > > DMA address 0 will probably go wrong at some point. So it's unlike > > kfree(NULL). > > I mean if skb allocation is successful in _dma_bd_init then in release path > we can assume .phys is always a valid address and skip rx_bd_v[i].cntrl > check. I don't think we can assume this. If the skb allocation succeeded, but then the dma_map_single failed (which we check with dma_mapping_error()), we would end up with a valid skb, but an uninitialised phys DMA address in the registers. That's why I set .cntrl only after having checked the dma_map_single() result. Or am I missing something? Cheers, Andre > > > > + dma_unmap_single(ndev->dev.parent, lp- > > > > >rx_bd_v[i].phys, > > > > + lp->max_frm_size, > > > > DMA_FROM_DEVICE); > > > > } > > > > + > > > > + dma_free_coherent(ndev->dev.parent, > > > > + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > > + lp->rx_bd_v, > > > > + lp->rx_bd_p); > > > > } > > > > > > > > /** > > > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device > > > > *ndev) > > > > sizeof(*lp->tx_bd_v) * lp- > > > > >tx_bd_num, > > > > &lp->tx_bd_p, GFP_KERNEL); > > > > if (!lp->tx_bd_v) > > > > - goto out; > > > > + return -ENOMEM; > > > > > > > > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, > > > > sizeof(*lp->rx_bd_v) * lp- > > > > >rx_bd_num, > > > > -- > > > > 2.17.1 > > > >
-----Original Message----- > From: Andre Przywara <andre.przywara@arm.com> > Sent: Thursday, January 16, 2020 11:33 PM > To: Radhey Shyam Pandey <radheys@xilinx.com> > Cc: David S . Miller <davem@davemloft.net>; Michal Simek > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>; > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup path > > On Fri, 10 Jan 2020 17:05:45 +0000 > Radhey Shyam Pandey <radheys@xilinx.com> wrote: > > Hi, > > > > -----Original Message----- > > > From: Andre Przywara <andre.przywara@arm.com> > > > Sent: Friday, January 10, 2020 9:13 PM > > > To: Radhey Shyam Pandey <radheys@xilinx.com> > > > Cc: David S . Miller <davem@davemloft.net>; Michal Simek > > > <michals@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>; > > > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > > > kernel@vger.kernel.org > > > Subject: Re: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup > > > path > > > > > > On Fri, 10 Jan 2020 15:14:46 +0000 > > > Radhey Shyam Pandey <radheys@xilinx.com> wrote: > > > > > > Hi Radhey, > > > > > > thanks for having a look! > > > > > > > > -----Original Message----- > > > > > From: Andre Przywara <andre.przywara@arm.com> > > > > > Sent: Friday, January 10, 2020 5:24 PM > > > > > To: David S . Miller <davem@davemloft.net>; Radhey Shyam Pandey > > > > > <radheys@xilinx.com> > > > > > Cc: Michal Simek <michals@xilinx.com>; Robert Hancock > > > > > <hancock@sedsystems.ca>; netdev@vger.kernel.org; linux-arm- > > > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > > > > Subject: [PATCH 03/14] net: axienet: Fix DMA descriptor cleanup > > > > > path > > > > > > > > > > When axienet_dma_bd_init() bails out during the initialisation > > > > > process, it might do so with parts of the structure already > > > > > allocated and initialised, while other parts have not been > > > > > touched yet. Before returning in this case, we call > > > > > axienet_dma_bd_release(), which does not take care of this corner case. > > > > > This is most obvious by the first loop happily dereferencing > > > > > lp->rx_bd_v, which we actually check to be non NULL *afterwards*. > > > > > > > > > > Make sure we only unmap or free already allocated structures, by: > > > > > - directly returning with -ENOMEM if nothing has been allocated > > > > > at all > > > > > - checking for lp->rx_bd_v to be non-NULL *before* using it > > > > > - only unmapping allocated DMA RX regions > > > > > > > > > > This avoids NULL pointer dereferences when initialisation fails. > > > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > --- > > > > > .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 > > > > > ++++++++++++------- > > > > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > > index 97482cf093ce..7e90044cf2d9 100644 > > > > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > > > > @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct > > > > > net_device *ndev) > > > > > int i; > > > > > struct axienet_local *lp = netdev_priv(ndev); > > > > > > > > > > + /* If we end up here, tx_bd_v must have been DMA allocated. > */ > > > > > + dma_free_coherent(ndev->dev.parent, > > > > > + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > > > + lp->tx_bd_v, > > > > > + lp->tx_bd_p); > > > > > + > > > > > + if (!lp->rx_bd_v) > > > > > + return; > > > > > + > > > > > for (i = 0; i < lp->rx_bd_num; i++) { > > > > > - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, > > > > > - lp->max_frm_size, DMA_FROM_DEVICE); > > > > > + /* A NULL skb means this descriptor has not been > initialised > > > > > + * at all. > > > > > + */ > > > > > + if (!lp->rx_bd_v[i].skb) > > > > > + break; > > > > > + > > > > > dev_kfree_skb(lp->rx_bd_v[i].skb); > > > > > - } > > > > > > > > > > - if (lp->rx_bd_v) { > > > > > - dma_free_coherent(ndev->dev.parent, > > > > > - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > > > - lp->rx_bd_v, > > > > > - lp->rx_bd_p); > > > > > - } > > > > > - if (lp->tx_bd_v) { > > > > > - dma_free_coherent(ndev->dev.parent, > > > > > - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, > > > > > - lp->tx_bd_v, > > > > > - lp->tx_bd_p); > > > > > + /* For each descriptor, we programmed cntrl with the > (non- > > > > > zero) > > > > > + * descriptor size, after it had been successfully > allocated. > > > > > + * So a non-zero value in there means we need to > unmap it. > > > > > + */ > > > > > > > > > + if (lp->rx_bd_v[i].cntrl) > > > > > > > > I think it should ok to unmap w/o any check? > > > > > > Do you mean because .phys would be 0 if not initialised? AFAIK 0 can > > > be a valid DMA address, so there is no special check for that, and > > > unmapping DMA address 0 will probably go wrong at some point. So > > > it's unlike kfree(NULL). > > > > I mean if skb allocation is successful in _dma_bd_init then in release > > path we can assume .phys is always a valid address and skip > > rx_bd_v[i].cntrl check. > > I don't think we can assume this. If the skb allocation succeeded, but then the > dma_map_single failed (which we check with dma_mapping_error()), we would > end up with a valid skb, but an uninitialised phys DMA address in the registers. > That's why I set .cntrl only after having checked the dma_map_single() result. > > Or am I missing something? Got it. Looks fine then. > > Cheers, > Andre > > > > > > + dma_unmap_single(ndev->dev.parent, lp- > > > > > >rx_bd_v[i].phys, > > > > > + lp->max_frm_size, > > > > > DMA_FROM_DEVICE); > > > > > } > > > > > + > > > > > + dma_free_coherent(ndev->dev.parent, > > > > > + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, > > > > > + lp->rx_bd_v, > > > > > + lp->rx_bd_p); > > > > > } > > > > > > > > > > /** > > > > > @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct > > > > > net_device > > > > > *ndev) > > > > > sizeof(*lp->tx_bd_v) * lp- > > > > > >tx_bd_num, > > > > > &lp->tx_bd_p, GFP_KERNEL); > > > > > if (!lp->tx_bd_v) > > > > > - goto out; > > > > > + return -ENOMEM; > > > > > > > > > > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, > > > > > sizeof(*lp->rx_bd_v) * lp- > > > > > >rx_bd_num, > > > > > -- > > > > > 2.17.1 > > > > > >
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 97482cf093ce..7e90044cf2d9 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -160,24 +160,37 @@ static void axienet_dma_bd_release(struct net_device *ndev) int i; struct axienet_local *lp = netdev_priv(ndev); + /* If we end up here, tx_bd_v must have been DMA allocated. */ + dma_free_coherent(ndev->dev.parent, + sizeof(*lp->tx_bd_v) * lp->tx_bd_num, + lp->tx_bd_v, + lp->tx_bd_p); + + if (!lp->rx_bd_v) + return; + for (i = 0; i < lp->rx_bd_num; i++) { - dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, - lp->max_frm_size, DMA_FROM_DEVICE); + /* A NULL skb means this descriptor has not been initialised + * at all. + */ + if (!lp->rx_bd_v[i].skb) + break; + dev_kfree_skb(lp->rx_bd_v[i].skb); - } - if (lp->rx_bd_v) { - dma_free_coherent(ndev->dev.parent, - sizeof(*lp->rx_bd_v) * lp->rx_bd_num, - lp->rx_bd_v, - lp->rx_bd_p); - } - if (lp->tx_bd_v) { - dma_free_coherent(ndev->dev.parent, - sizeof(*lp->tx_bd_v) * lp->tx_bd_num, - lp->tx_bd_v, - lp->tx_bd_p); + /* For each descriptor, we programmed cntrl with the (non-zero) + * descriptor size, after it had been successfully allocated. + * So a non-zero value in there means we need to unmap it. + */ + if (lp->rx_bd_v[i].cntrl) + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys, + lp->max_frm_size, DMA_FROM_DEVICE); } + + dma_free_coherent(ndev->dev.parent, + sizeof(*lp->rx_bd_v) * lp->rx_bd_num, + lp->rx_bd_v, + lp->rx_bd_p); } /** @@ -207,7 +220,7 @@ static int axienet_dma_bd_init(struct net_device *ndev) sizeof(*lp->tx_bd_v) * lp->tx_bd_num, &lp->tx_bd_p, GFP_KERNEL); if (!lp->tx_bd_v) - goto out; + return -ENOMEM; lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent, sizeof(*lp->rx_bd_v) * lp->rx_bd_num,
When axienet_dma_bd_init() bails out during the initialisation process, it might do so with parts of the structure already allocated and initialised, while other parts have not been touched yet. Before returning in this case, we call axienet_dma_bd_release(), which does not take care of this corner case. This is most obvious by the first loop happily dereferencing lp->rx_bd_v, which we actually check to be non NULL *afterwards*. Make sure we only unmap or free already allocated structures, by: - directly returning with -ENOMEM if nothing has been allocated at all - checking for lp->rx_bd_v to be non-NULL *before* using it - only unmapping allocated DMA RX regions This avoids NULL pointer dereferences when initialisation fails. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- .../net/ethernet/xilinx/xilinx_axienet_main.c | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-)