All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Abeni <pabeni@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>, Hawkins Jiawei <yin31149@gmail.com>
Cc: 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 19:57:10 +0100	[thread overview]
Message-ID: <bc4616002932b25973533c39c07f48ea57afa3dc.camel@redhat.com> (raw)
In-Reply-To: <20221115090237.5d5988bb@kernel.org>

On Tue, 2022-11-15 at 09:02 -0800, Jakub Kicinski wrote:
> On Mon, 14 Nov 2022 01:05:08 +0800 Hawkins Jiawei wrote:
> 
> > @@ -479,6 +480,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> >  	}
> >  
> >  	if (old_r && old_r != r) {
> > +		old_e = old_r->exts;
> >  		err = tcindex_filter_result_init(old_r, cp, net);
> >  		if (err < 0) {
> >  			kfree(f);
> > @@ -510,6 +512,12 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> >  		tcf_exts_destroy(&new_filter_result.exts);
> >  	}
> >  
> > +	/* Note: old_e should be destroyed after the RCU grace period,
> > +	 * to avoid possible use-after-free by concurrent readers.
> > +	 */
> > +	synchronize_rcu();
> > +	tcf_exts_destroy(&old_e);
> 
> I don't think this dance is required, @cp is a copy of the original
> data, and the original (@p) is destroyed in a safe manner below.

This code confuses me more than a bit, and I don't follow ?!? 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, 

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?

Paolo


  reply	other threads:[~2022-11-15 18:58 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 [this message]
2022-11-16  2:44     ` Jakub Kicinski
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=bc4616002932b25973533c39c07f48ea57afa3dc.camel@redhat.com \
    --to=pabeni@redhat.com \
    --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=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --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.