Re: [PATCH v2 net-next 3/4] rocker: do not make neighbour entry changes when preparing transactions

[Simon Horman <simon.horman@netronome.com>]

On Tue, May 19, 2015 at 08:16:04PM -0700, Scott Feldman wrote:
> On Tue, May 19, 2015 at 7:00 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> >
> > rocker_port_ipv4_nh() and in turn rocker_port_ipv4_neigh() may be
> > be called with trans == SWITCHDEV_TRANS_PREPARE and then
> > trans == SWITCHDEV_TRANS_COMMIT from switchdev_port_obj_set() via
> > fib_table_insert().
> >
> > The first time that rocker_port_ipv4_nh() is called, with
> > trans == SWITCHDEV_TRANS_PREPARE, _rocker_neigh_add() adds a new entry to
> > the neigh table.
> >
> > And the second time  rocker_port_ipv4_nh() is called, with
> > trans == SWITCHDEV_TRANS_COMMIT, that entry is found. This causes
> > rocker_port_ipv4_nh() to believe it is not adding an entry and thus it
> > frees "entry", which is still present in rocker driver's neigh table.
> >
> > This problem does not appear to affect deletion as my analysis is that
> > deletion is always performed with trans == SWITCHDEV_TRANS_NONE.
> >
> > For completeness _rocker_neigh_{add,del,prepare} are updated not to
> > manipulate fib table entries if trans == SWITCHDEV_TRANS_PREPARE.
> >
> > Fixes: c4f20321d968 ("rocker: support prepare-commit transaction model")
> > Reported-by: oshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >
> > ---
> > v2
> > * As suggested by Scott Feldman, only guard ref_count adjustment
> >   with (trans == SWITCHDEV_TRANS_PREPARE) in _rocker_neigh_update.
> > * Updated changelog to note that an entry is freed but left
> >   in the neigh table. Thanks to Toshiaki Makita.
> > ---
> >  drivers/net/ethernet/rocker/rocker.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> > index 89d22bdcbdc4..b836dd315e8a 100644
> > --- a/drivers/net/ethernet/rocker/rocker.c
> > +++ b/drivers/net/ethernet/rocker/rocker.c
> > @@ -2909,8 +2909,11 @@ static struct rocker_neigh_tbl_entry *
> >  }
> >
> >  static void _rocker_neigh_add(struct rocker *rocker,
> > +                             enum switchdev_trans trans,
> >                               struct rocker_neigh_tbl_entry *entry)
> >  {
> > +       if (trans == SWITCHDEV_TRANS_PREPARE)
> > +               return;
> >         entry->index = rocker->neigh_tbl_next_index++;
> >         entry->ref_count++;
> >         hash_add(rocker->neigh_tbl, &entry->entry,
> > @@ -2921,6 +2924,8 @@ static void _rocker_neigh_del(struct rocker_port *rocker_port,
> >                               enum switchdev_trans trans,
> >                               struct rocker_neigh_tbl_entry *entry)
> >  {
> > +       if (trans == SWITCHDEV_TRANS_PREPARE)
> > +               return;
> >         if (--entry->ref_count == 0) {
> >                 hash_del(&entry->entry);
> >                 rocker_port_kfree(rocker_port, trans, entry);
> > @@ -2928,12 +2933,13 @@ static void _rocker_neigh_del(struct rocker_port *rocker_port,
> >  }
> >
> >  static void _rocker_neigh_update(struct rocker_neigh_tbl_entry *entry,
> > +                                enum switchdev_trans trans,
> >                                  u8 *eth_dst, bool ttl_check)
> >  {
> >         if (eth_dst) {
> >                 ether_addr_copy(entry->eth_dst, eth_dst);
> >                 entry->ttl_check = ttl_check;
> > -       } else {
> > +       } else if (trans == SWITCHDEV_TRANS_PREPARE) {
> >                 entry->ref_count++;
> >         }
> 
> 
> hmmm...should that be else if (trans != SWITCHDEV_TRANS_PREPARE)?

Yes, sorry about that.

> If so, please send v3 and add my Acked-by.

Thanks, will do.