* refcount issue with net/mlx5: DR, Expose steering table functionality
@ 2019-12-18 15:54 Marcelo Ricardo Leitner
2019-12-23 22:12 ` Saeed Mahameed
0 siblings, 1 reply; 2+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-18 15:54 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: netdev, Alex Vesker, Erez Shitrit, Mark Bloch, Guy Twig
Hi,
(using a reply to the original patch as base for this email)
On Tue, Sep 03, 2019 at 08:04:44PM +0000, Saeed Mahameed wrote:
...
> +static int dr_table_init_nic(struct mlx5dr_domain *dmn,
> + struct mlx5dr_table_rx_tx *nic_tbl)
> +{
> + struct mlx5dr_domain_rx_tx *nic_dmn = nic_tbl->nic_dmn;
> + struct mlx5dr_htbl_connect_info info;
> + int ret;
> +
> + nic_tbl->default_icm_addr = nic_dmn->default_icm_addr;
> +
> + nic_tbl->s_anchor = mlx5dr_ste_htbl_alloc(dmn->ste_icm_pool,
> + DR_CHUNK_SIZE_1,
> + MLX5DR_STE_LU_TYPE_DONT_CARE,
> + 0);
[A]
> + if (!nic_tbl->s_anchor)
> + return -ENOMEM;
> +
> + info.type = CONNECT_MISS;
> + info.miss_icm_addr = nic_dmn->default_icm_addr;
> + ret = mlx5dr_ste_htbl_init_and_postsend(dmn, nic_dmn,
> + nic_tbl->s_anchor,
> + &info, true);
> + if (ret)
> + goto free_s_anchor;
> +
> + mlx5dr_htbl_get(nic_tbl->s_anchor);
We have an issue here. mlx5dr_ste_htbl_alloc() above in [A] will:
refcount_set(&htbl->refcount, 0);
and then, if no error happens, here it gets incremented. But:
static inline void mlx5dr_htbl_get(struct mlx5dr_ste_htbl *htbl)
{
refcount_inc(&htbl->refcount);
* Will WARN if the refcount is 0, as this represents a possible use-after-free
* condition.
*/
static inline void refcount_inc(refcount_t *r)
{
refcount_add(1, r);
and that's exactly what happens here (commit 2187f215ebaac73ddbd814696d7c7fa34f0c3de0):
[ 163.379526] mlx5_core 0000:82:00.0: E-Switch: Disable: mode(LEGACY), nvfs(4), active vports(5)
[ 166.862171] ------------[ cut here ]------------
[ 166.867331] refcount_t: addition on 0; use-after-free.
[ 166.873094] WARNING: CPU: 49 PID: 5414 at lib/refcount.c:25 refcount_warn_saturate+0x6d/0xf0
[ 166.882511] Kernel panic - not syncing: panic_on_warn set ...
[ 166.888923] CPU: 49 PID: 5414 Comm: devlink Kdump: loaded Not tainted 5.5.0-rc2+ #2
...
[ 166.955337] RIP: 0010:refcount_warn_saturate+0x6d/0xf0
...
[ 167.027666] ? refcount_warn_saturate+0x6d/0xf0
[ 167.032772] dr_table_init_nic+0xd1/0xe0 [mlx5_core]
[ 167.038339] mlx5dr_table_create+0x12e/0x260 [mlx5_core]
[ 167.044290] mlx5_cmd_dr_create_flow_table+0x31/0xd0 [mlx5_core]
[ 167.051013] __mlx5_create_flow_table+0x222/0x680 [mlx5_core]
[ 167.057450] esw_create_offloads_fdb_tables+0x169/0x4c0 [mlx5_core]
[ 167.064468] esw_offloads_enable+0x16c/0x510 [mlx5_core]
[ 167.070417] ? mlx5_add_device+0x9d/0xe0 [mlx5_core]
[ 167.075982] mlx5_eswitch_enable+0xf9/0x4f0 [mlx5_core]
[ 167.081838] mlx5_devlink_eswitch_mode_set+0x11b/0x1b0 [mlx5_core]
[ 167.088738] devlink_nl_cmd_eswitch_set_doit+0x44/0xc0
[ 167.094466] genl_rcv_msg+0x1f9/0x440
[ 167.098545] ? genl_family_rcv_msg_attrs_parse+0x110/0x110
[ 167.104666] netlink_rcv_skb+0x49/0x110
[ 167.108945] genl_rcv+0x24/0x40
[ 167.112449] netlink_unicast+0x1a5/0x280
[ 167.116825] netlink_sendmsg+0x23d/0x470
[ 167.121202] sock_sendmsg+0x5b/0x60
[ 167.125093] __sys_sendto+0xee/0x160
One quick fix is to just initialize it as 1. I was sketching a patch
but gave up as mlx5dr_ste_htbl_alloc() also does:
refcount_set(&ste->refcount, 0);
and it is used like:
bool mlx5dr_ste_not_used_ste(struct mlx5dr_ste *ste)
{
return !refcount_read(&ste->refcount);
So the same easy fix doesn't work here and:
static inline void mlx5dr_ste_put(struct mlx5dr_ste *ste,
struct mlx5dr_matcher *matcher,
struct mlx5dr_matcher_rx_tx *nic_matcher)
{
if (refcount_dec_and_test(&ste->refcount))
mlx5dr_ste_free(ste, matcher, nic_matcher);
On which, AFAICT, removes it from the HW but not free the STE entry
itself. So I think that the usage of refcount_dec_and_test() would be
broken here if we just offset the refcount by 1.
Thoughts?
> +
> + return 0;
> +
> +free_s_anchor:
> + mlx5dr_ste_htbl_free(nic_tbl->s_anchor);
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: refcount issue with net/mlx5: DR, Expose steering table functionality
2019-12-18 15:54 refcount issue with net/mlx5: DR, Expose steering table functionality Marcelo Ricardo Leitner
@ 2019-12-23 22:12 ` Saeed Mahameed
0 siblings, 0 replies; 2+ messages in thread
From: Saeed Mahameed @ 2019-12-23 22:12 UTC (permalink / raw)
To: marcelo.leitner; +Cc: Guy Twig, Mark Bloch, netdev, Alex Vesker, Erez Shitrit
On Wed, 2019-12-18 at 12:54 -0300, Marcelo Ricardo Leitner wrote:
> Hi,
>
> (using a reply to the original patch as base for this email)
>
> On Tue, Sep 03, 2019 at 08:04:44PM +0000, Saeed Mahameed wrote:
> ...
> > +static int dr_table_init_nic(struct mlx5dr_domain *dmn,
> > + struct mlx5dr_table_rx_tx *nic_tbl)
> > +{
> > + struct mlx5dr_domain_rx_tx *nic_dmn = nic_tbl->nic_dmn;
> > + struct mlx5dr_htbl_connect_info info;
> > + int ret;
> > +
> > + nic_tbl->default_icm_addr = nic_dmn->default_icm_addr;
> > +
> > + nic_tbl->s_anchor = mlx5dr_ste_htbl_alloc(dmn->ste_icm_pool,
> > + DR_CHUNK_SIZE_1,
> > + MLX5DR_STE_LU_TYPE_DO
> > NT_CARE,
> > + 0);
>
> [A]
>
> > + if (!nic_tbl->s_anchor)
> > + return -ENOMEM;
> > +
> > + info.type = CONNECT_MISS;
> > + info.miss_icm_addr = nic_dmn->default_icm_addr;
> > + ret = mlx5dr_ste_htbl_init_and_postsend(dmn, nic_dmn,
> > + nic_tbl->s_anchor,
> > + &info, true);
> > + if (ret)
> > + goto free_s_anchor;
> > +
> > + mlx5dr_htbl_get(nic_tbl->s_anchor);
>
> We have an issue here. mlx5dr_ste_htbl_alloc() above in [A] will:
> refcount_set(&htbl->refcount, 0);
> and then, if no error happens, here it gets incremented. But:
>
> static inline void mlx5dr_htbl_get(struct mlx5dr_ste_htbl *htbl)
> {
> refcount_inc(&htbl->refcount);
>
> * Will WARN if the refcount is 0, as this represents a possible use-
> after-free
> * condition.
> */
> static inline void refcount_inc(refcount_t *r)
> {
> refcount_add(1, r);
>
> and that's exactly what happens here (commit
> 2187f215ebaac73ddbd814696d7c7fa34f0c3de0):
> [ 163.379526] mlx5_core 0000:82:00.0: E-Switch: Disable:
> mode(LEGACY), nvfs(4), active vports(5)
> [ 166.862171] ------------[ cut here ]------------
> [ 166.867331] refcount_t: addition on 0; use-after-free.
> [ 166.873094] WARNING: CPU: 49 PID: 5414 at lib/refcount.c:25
> refcount_warn_saturate+0x6d/0xf0
> [ 166.882511] Kernel panic - not syncing: panic_on_warn set ...
> [ 166.888923] CPU: 49 PID: 5414 Comm: devlink Kdump: loaded Not
> tainted 5.5.0-rc2+ #2
> ...
> [ 166.955337] RIP: 0010:refcount_warn_saturate+0x6d/0xf0
> ...
> [ 167.027666] ? refcount_warn_saturate+0x6d/0xf0
> [ 167.032772] dr_table_init_nic+0xd1/0xe0 [mlx5_core]
> [ 167.038339] mlx5dr_table_create+0x12e/0x260 [mlx5_core]
> [ 167.044290] mlx5_cmd_dr_create_flow_table+0x31/0xd0 [mlx5_core]
> [ 167.051013] __mlx5_create_flow_table+0x222/0x680 [mlx5_core]
> [ 167.057450] esw_create_offloads_fdb_tables+0x169/0x4c0
> [mlx5_core]
> [ 167.064468] esw_offloads_enable+0x16c/0x510 [mlx5_core]
> [ 167.070417] ? mlx5_add_device+0x9d/0xe0 [mlx5_core]
> [ 167.075982] mlx5_eswitch_enable+0xf9/0x4f0 [mlx5_core]
> [ 167.081838] mlx5_devlink_eswitch_mode_set+0x11b/0x1b0 [mlx5_core]
> [ 167.088738] devlink_nl_cmd_eswitch_set_doit+0x44/0xc0
> [ 167.094466] genl_rcv_msg+0x1f9/0x440
> [ 167.098545] ? genl_family_rcv_msg_attrs_parse+0x110/0x110
> [ 167.104666] netlink_rcv_skb+0x49/0x110
> [ 167.108945] genl_rcv+0x24/0x40
> [ 167.112449] netlink_unicast+0x1a5/0x280
> [ 167.116825] netlink_sendmsg+0x23d/0x470
> [ 167.121202] sock_sendmsg+0x5b/0x60
> [ 167.125093] __sys_sendto+0xee/0x160
>
> One quick fix is to just initialize it as 1. I was sketching a patch
> but gave up as mlx5dr_ste_htbl_alloc() also does:
> refcount_set(&ste->refcount, 0);
>
> and it is used like:
> bool mlx5dr_ste_not_used_ste(struct mlx5dr_ste *ste)
> {
> return !refcount_read(&ste->refcount);
>
> So the same easy fix doesn't work here and:
>
> static inline void mlx5dr_ste_put(struct mlx5dr_ste *ste,
> struct mlx5dr_matcher *matcher,
> struct mlx5dr_matcher_rx_tx
> *nic_matcher)
> {
> if (refcount_dec_and_test(&ste->refcount))
> mlx5dr_ste_free(ste, matcher, nic_matcher);
>
> On which, AFAICT, removes it from the HW but not free the STE entry
> itself. So I think that the usage of refcount_dec_and_test() would be
> broken here if we just offset the refcount by 1.
>
> Thoughts?
>
Hi Marcelo, thanks for the report,
We are working on a patch to remove the use of refcount API in mlx5 DR,
since it is redundant and all updates are already done under a global
lock for now.
I will CC you on that patch prior to submission.
Thanks,
Saeed.
> > +
> > + return 0;
> > +
> > +free_s_anchor:
> > + mlx5dr_ste_htbl_free(nic_tbl->s_anchor);
> > + return ret;
> > +}
> > +
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-12-23 22:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 15:54 refcount issue with net/mlx5: DR, Expose steering table functionality Marcelo Ricardo Leitner
2019-12-23 22:12 ` Saeed Mahameed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).