Re: [PATCH net-next v2 1/6] bnge: Extract bnapi allocation and cleanup into helpers

[Bhargava Chenna Marreddy]

> > > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > > index a4288f0258f87..c1810a3098883 100644
> > > --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> > > @@ -825,6 +825,9 @@ static void bnge_free_tpa_info(struct bnge_net *bn)
> > >     struct bnge_dev *bd = bn->bd;
> > >     int i, j;
> > >
> > > +   if (!bn->rx_ring)
> > > +           return;
> > > +
>
> The check above appears to be purely defensive.
> I do not believe it can ever be true.
> If so, please remove it.

Hi Simon,

Ack, will fix in next version.

> > > @@ -1024,6 +1030,9 @@ static void bnge_free_tx_rings(struct bnge_net *bn)
> > >     struct bnge_dev *bd = bn->bd;
> > >     int i;
> > >
> > > +   if (!bn->tx_ring)
> > > +           return;
> > > +
> >
> > [Severity: Medium]
> > Are these three new NULL checks really just preparation, or do they fix a
> > reachable NULL pointer dereference that exists today?
>
> It seems to me that the two hunks above do fix a bug,
> can could manifest under memory pressure..
> And thus should be broken out as a separate patch for net.

Agreed, will send as a separate net fix.

> > >     bn->rx_ring = kzalloc_objs(struct bnge_rx_ring_info, bd->rx_nr_rings);
> > >     if (!bn->rx_ring)
> > >             goto err_free_core;
> >
> > This is the branch that reaches bnge_free_core() with both bn->rx_ring and
> > bn->tx_ring still NULL.
>
> I will add that they way that this code relies on unwind functions,
> such as bnge_free_core(), that can unwind from both a fully and partially
> initialised state is not the preferred approach.
>
> E.g. while having bnge_free_core() fully clean up state the taredown
> patch is fine, the probe path should have a goto ladder that unwinds
> incrementally based on where initialization failed.
>
> For a bug fix, a minimal approach, that adds some conditions, is probably
> best. But perhaps there are more cases than are covered by this discussion.
>

Thanks. I found no other cases in the helpers called from bnge_free_core().
I plan to send a goto-ladder fix for bnge_alloc_core() to net.

Thanks,
Bhargava Marreddy

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature