netdev - Re: refcount issue with net/mlx5: DR, Expose steering table functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a7f02d224e6a4ce568691d623662669d12749291.camel@mellanox.com>
Date:   Mon, 23 Dec 2019 22:12:52 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "marcelo.leitner@...il.com" <marcelo.leitner@...il.com>
CC:     Guy Twig <guyt@...lanox.com>, Mark Bloch <markb@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alex Vesker <valex@...lanox.com>,
        Erez Shitrit <erezsh@...lanox.com>
Subject: Re: refcount issue with net/mlx5: DR, Expose steering table
 functionality

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;
> > +}
> > +