Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 5 Aug 2019 09:13:20 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Chuhong Yuan <hslester96@gmail.com>
Cc:     Saeed Mahameed <saeedm@mellanox.com>,
        "David S . Miller" <davem@davemloft.net>,
        Netdev <netdev@vger.kernel.org>, linux-rdma@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] net/mlx5e: Use refcount_t for refcount
Message-ID: <20190805061320.GN4832@mtr-leonro.mtl.com>
References: <20190802164828.20243-1-hslester96@gmail.com>
 <20190804125858.GJ4832@mtr-leonro.mtl.com>
 <CANhBUQ2H5MU0m2xM0AkJGPf7+MJBZ3Eq5rR0kgeOoKRi4q1j6Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANhBUQ2H5MU0m2xM0AkJGPf7+MJBZ3Eq5rR0kgeOoKRi4q1j6Q@mail.gmail.com>
User-Agent: Mutt/1.12.0 (2019-05-25)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Aug 04, 2019 at 10:44:47PM +0800, Chuhong Yuan wrote:
> On Sun, Aug 4, 2019 at 8:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sat, Aug 03, 2019 at 12:48:28AM +0800, Chuhong Yuan wrote:
> > > refcount_t is better for reference counters since its
> > > implementation can prevent overflows.
> > > So convert atomic_t ref counters to refcount_t.
> >
> > I'm not thrilled to see those automatic conversion patches, especially
> > for flows which can't overflow. There is nothing wrong in using atomic_t
> > type of variable, do you have in mind flow which will cause to overflow?
> >
> > Thanks
>
> I have to say that these patches are not done automatically...
> Only the detection of problems is done by a script.
> All conversions are done manually.

Even worse, you need to audit usage of atomic_t and replace there
it can overflow.

>
> I am not sure whether the flow can cause an overflow.

It can't.

> But I think it is hard to ensure that a data path is impossible
> to have problems in any cases including being attacked.

It is not data path, and I doubt that such conversion will be allowed
in data paths without proving that no performance regression is introduced.

>
> So I think it is better to do this minor revision to prevent
> potential risk, just like we have done in mlx5/core/cq.c.

mlx5/core/cq.c is a different beast, refcount there means actual users
of CQ which are limited in SW, so in theory, they have potential
to be overflown.

It is not the case here, there your are adding new port.
There is nothing wrong with atomic_t.

Thanks

>
> Regards,
> Chuhong
>
> > >
> > > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > > ---
> > > Changes in v2:
> > >   - Add #include.
> > >
> > >  drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > index b9d4f4e19ff9..148b55c3db7a 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/vxlan.c
> > > @@ -32,6 +32,7 @@
> > >
> > >  #include <linux/kernel.h>
> > >  #include <linux/module.h>
> > > +#include <linux/refcount.h>
> > >  #include <linux/mlx5/driver.h>
> > >  #include <net/vxlan.h>
> > >  #include "mlx5_core.h"
> > > @@ -48,7 +49,7 @@ struct mlx5_vxlan {
> > >
> > >  struct mlx5_vxlan_port {
> > >       struct hlist_node hlist;
> > > -     atomic_t refcount;
> > > +     refcount_t refcount;
> > >       u16 udp_port;
> > >  };
> > >
> > > @@ -113,7 +114,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
> > >
> > >       vxlanp = mlx5_vxlan_lookup_port(vxlan, port);
> > >       if (vxlanp) {
> > > -             atomic_inc(&vxlanp->refcount);
> > > +             refcount_inc(&vxlanp->refcount);
> > >               return 0;
> > >       }
> > >
> > > @@ -137,7 +138,7 @@ int mlx5_vxlan_add_port(struct mlx5_vxlan *vxlan, u16 port)
> > >       }
> > >
> > >       vxlanp->udp_port = port;
> > > -     atomic_set(&vxlanp->refcount, 1);
> > > +     refcount_set(&vxlanp->refcount, 1);
> > >
> > >       spin_lock_bh(&vxlan->lock);
> > >       hash_add(vxlan->htable, &vxlanp->hlist, port);
> > > @@ -170,7 +171,7 @@ int mlx5_vxlan_del_port(struct mlx5_vxlan *vxlan, u16 port)
> > >               goto out_unlock;
> > >       }
> > >
> > > -     if (atomic_dec_and_test(&vxlanp->refcount)) {
> > > +     if (refcount_dec_and_test(&vxlanp->refcount)) {
> > >               hash_del(&vxlanp->hlist);
> > >               remove = true;
> > >       }
> > > --
> > > 2.20.1
> > >