netdev - Re: [PATCH v5 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIEqAosXPn8hB1hK@C02FL77VMD6R.googleapis.com>
Date: Wed, 7 Jun 2023 18:08:18 -0700
From: Peilin Ye <yepeilin.cs@...il.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Jakub Kicinski <kuba@...nel.org>,
	Pedro Tammela <pctammela@...atatu.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	Peilin Ye <peilin.ye@...edance.com>,
	Daniel Borkmann <daniel@...earbox.net>,
	John Fastabend <john.fastabend@...il.com>,
	Hillf Danton <hdanton@...a.com>, netdev@...r.kernel.org,
	Cong Wang <cong.wang@...edance.com>
Subject: Re: [PATCH v5 net 6/6] net/sched: qdisc_destroy() old ingress and
 clsact Qdiscs before grafting

On Wed, Jun 07, 2023 at 11:18:32AM +0300, Vlad Buslov wrote:
> > I also thought about adding the new DELETED-REJECT-NEW-FILTERS flag to
> > ::state2, but not sure if it's okay to extend it for our purpose.
>
> As you described above qdisc->flags is already used to interact with cls
> api (including changing it dynamically), so I don't see why not.

Sorry, I don't follow, I meant qdisc->state2:

  enum qdisc_state2_t {
          /* Only for !TCQ_F_NOLOCK qdisc. Never access it directly.
           * Use qdisc_run_begin/end() or qdisc_is_running() instead.
           */
          __QDISC_STATE2_RUNNING,
  };

NVM, I think using qdisc->flags after making it atomic sounds better.

On Wed, Jun 07, 2023 at 11:18:32AM +0300, Vlad Buslov wrote:
> > 	err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
> > 			      flags, extack);
> > 	if (err == 0) {
> > 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
> > 			       RTM_NEWTFILTER, false, rtnl_held, extack);
> > 		tfilter_put(tp, fh);
> > 		/* q pointer is NULL for shared blocks */
> > 		if (q)
> > 			q->flags &= ~TCQ_F_CAN_BYPASS;
> > 	}               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > TCQ_F_CAN_BYPASS is cleared after e.g. adding a filter to the Qdisc, and it
> > isn't atomic [1].
> 
> Yeah, I see we have already got such behavior in 3f05e6886a59
> ("net_sched: unset TCQ_F_CAN_BYPASS when adding filters").
> 
> > We also have this:
> >
> >   ->dequeue()
> >     htb_dequeue()
> >       htb_dequeue_tree()
> >         qdisc_warn_nonwc():
> >
> >   void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
> >   {
> >           if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
> >                   pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
> >                           txt, qdisc->ops->id, qdisc->handle >> 16);
> >                   qdisc->flags |= TCQ_F_WARN_NONWC;
> >           }       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   }
> >   EXPORT_SYMBOL(qdisc_warn_nonwc);
> >
> > Also non-atomic; isn't it possible for the above 2 underlined statements to
> > race with each other?  If true, I think we need to change Qdisc::flags to
> > use atomic bitops, just like what we're doing for Qdisc::state and
> > ::state2.  It feels like a separate TODO, however.
> 
> It looks like even though 3f05e6886a59 ("net_sched: unset
> TCQ_F_CAN_BYPASS when adding filters") was introduced after cls api
> unlock by now we have these in exactly the same list of supported
> kernels (5.4 LTS and newer). Considering this, the conversion to the
> atomic bitops can be done as a standalone fix for cited commit and after
> it will have been accepted and backported the qdisc fix can just assume
> that qdisc->flags is an atomic bitops field in all target kernels and
> use it as-is. WDYT?

Sounds great, how about:

  1. I'll post the non-replay version of the fix (after updating the commit
     message), and we apply that first, as suggested by Jamal

  2. Make qdisc->flags atomic

  3. Make the fix better by replaying and using the (now atomic)
     IS-DESTROYING flag with test_and_set_bit() and friends

?

Thanks,
Peilin Ye