From: Daniel Borkmann <daniel@iogearbox•net>
To: Saeed Mahameed <saeedm@dev•mellanox.co.il>
Cc: "David S. Miller" <davem@davemloft•net>,
Alexei Starovoitov <alexei.starovoitov@gmail•com>,
Brenden Blanco <bblanco@plumgrid•com>,
zhiyisun@gmail•com, Rana Shahout <ranas@mellanox•com>,
Saeed Mahameed <saeedm@mellanox•com>,
Linux Netdev List <netdev@vger•kernel.org>
Subject: Re: [PATCH net-next v2 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
Date: Wed, 16 Nov 2016 16:55:38 +0100 [thread overview]
Message-ID: <582C817A.4030902@iogearbox.net> (raw)
In-Reply-To: <582C7F11.7000006@iogearbox.net>
On 11/16/2016 04:45 PM, Daniel Borkmann wrote:
> On 11/16/2016 01:51 PM, Saeed Mahameed wrote:
>> On Wed, Nov 16, 2016 at 2:04 AM, Daniel Borkmann <daniel@iogearbox•net> wrote:
>>> mlx5e_xdp_set() is currently the only place where we drop reference on the
>>> prog sitting in priv->xdp_prog when it's exchanged by a new one. We also
>>> need to make sure that we eventually release that reference, for example,
>>> in case the netdev is dismantled.
>>>
>>> Fixes: 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")
>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox•net>
>>> ---
>>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> index cf26672..60fe54c 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> @@ -3715,6 +3715,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
>>>
>>> if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>> mlx5_eswitch_unregister_vport_rep(esw, 0);
>>> +
>>> + if (priv->xdp_prog)
>>> + bpf_prog_put(priv->xdp_prog);
>>> }
>>
>> I thought that on unregister_netdev ndo_xdp_set will be called with
>> NULL prog to cleanup. like any other resources (Vlans/mac_lists/
>> etc..), why xdp should be different ?
>> Anyway if this is the case, I am ok with this fix, you can even send
>> it to net (it looks like a serious leak).
>
> The only interaction with ndo_xdp() right now is dev_change_xdp_fd()
> and the currently a bit terse dump via rtnl_xdp_fill(). The latter
> only tells whether something is actually attached and will have more
> info in near future, but doesn't alter anything.
>
> dev_change_xdp_fd() is only triggered from user side via netlink when
> IFLA_XDP container attr is around, so no automatic cleanup here. This
> means as per documentation in enum xdp_netdev_command, that the driver
> has full ownership, thus needs to bpf_prog_put().
Note that without patch 2/4, just sending this one to net doesn't really
solve anything, since there the mlx5e_xdp_set() still has the incorrect
bpf_prog_add(prog, 1) around. So it's the whole series if so. I had it
originally targeted at net, but Alexei suggested net-next; I don't really
mind either way, so I agreed to go for net-next.
next prev parent reply other threads:[~2016-11-16 15:55 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-16 0:04 [PATCH net-next v2 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
2016-11-16 0:04 ` [PATCH net-next v2 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
2016-11-16 0:04 ` [PATCH net-next v2 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set Daniel Borkmann
2016-11-16 12:25 ` Saeed Mahameed
2016-11-16 13:54 ` Daniel Borkmann
2016-11-16 14:30 ` Saeed Mahameed
2016-11-16 15:26 ` Daniel Borkmann
2016-11-17 9:48 ` Saeed Mahameed
2016-11-17 20:03 ` Daniel Borkmann
2016-11-16 0:04 ` [PATCH net-next v2 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
2016-11-16 12:51 ` Saeed Mahameed
2016-11-16 15:45 ` Daniel Borkmann
2016-11-16 15:55 ` Daniel Borkmann [this message]
2016-11-17 9:34 ` Saeed Mahameed
2016-11-16 0:04 ` [PATCH net-next v2 4/4] bpf: add __must_check attributes to refcount manipulating helpers Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=582C817A.4030902@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=bblanco@plumgrid$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ranas@mellanox$(echo .)com \
--cc=saeedm@dev$(echo .)mellanox.co.il \
--cc=saeedm@mellanox$(echo .)com \
--cc=zhiyisun@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox