All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Hawkins Jiawei <yin31149@gmail.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	18801353760@163.com,
	syzbot+232ebdbd36706c965ebf@syzkaller.appspotmail.com,
	syzkaller-bugs@googlegroups.com,
	Cong Wang <cong.wang@bytedance.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: sched: fix memory leak in tcindex_set_parms
Date: Tue, 15 Nov 2022 18:44:42 -0800	[thread overview]
Message-ID: <20221115184442.272b6ea8@kernel.org> (raw)
In-Reply-To: <bc4616002932b25973533c39c07f48ea57afa3dc.camel@redhat.com>

On Tue, 15 Nov 2022 19:57:10 +0100 Paolo Abeni wrote:
> This code confuses me more than a bit, and I don't follow ?!? 

It's very confusing :S

For starters I don't know when r != old_r. I mean now it triggers
randomly after the RCU-ification, but in the original code when
it was just a memset(). When would old_r ever not be null and yet
point to a different entry?

> it looks like that at this point:
> 
> * the data path could access 'old_r->exts' contents via 'p' just before
> the previous 'tcindex_filter_result_init(old_r, cp, net);' but still
> potentially within the same RCU grace period
> 
> * 'tcindex_filter_result_init(old_r, cp, net);' has 'unlinked' the old
> exts from 'p'  so that will not be freed by later
> tcindex_partial_destroy_work() 
> 
> Overall it looks to me that we need some somewhat wait for the RCU
> grace period, 

Isn't it better to make @cp a deeper copy of @p ?
I thought it already is but we don't seem to be cloning p->h.
Also the cloning of p->perfect looks quite lossy.

> Somewhat side question: it looks like that the 'perfect hashing' usage
> is the root cause of the issue addressed here, and very likely is
> afflicted by other problems, e.g. the data curruption in 'err =
> tcindex_filter_result_init(old_r, cp, net);'.
> 
> AFAICS 'perfect hashing' usage is a sort of optimization that the user-
> space may trigger with some combination of the tcindex arguments. I'm
> wondering if we could drop all perfect hashing related code?

The thought of "how much of this can we delete" did cross my mind :)

  reply	other threads:[~2022-11-16  2:44 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-13 17:05 [PATCH v2] net: sched: fix memory leak in tcindex_set_parms Hawkins Jiawei
2022-11-15 11:36 ` Paolo Abeni
2022-11-15 11:39   ` Dmitry Vyukov
2022-11-15 17:02 ` Jakub Kicinski
2022-11-15 18:57   ` Paolo Abeni
2022-11-16  2:44     ` Jakub Kicinski [this message]
2022-11-16 12:10       ` Hawkins Jiawei
2022-11-16 13:21         ` Hawkins Jiawei
2022-11-16  7:35   ` Hawkins Jiawei

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=20221115184442.272b6ea8@kernel.org \
    --to=kuba@kernel.org \
    --cc=18801353760@163.com \
    --cc=cong.wang@bytedance.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=syzbot+232ebdbd36706c965ebf@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=xiyou.wangcong@gmail.com \
    --cc=yin31149@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.