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: <87y1kvwu5c.fsf@nvidia.com>
Date: Wed, 7 Jun 2023 11:18:32 +0300
From: Vlad Buslov <vladbu@...dia.com>
To: Peilin Ye <yepeilin.cs@...il.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 Tue 06 Jun 2023 at 17:57, Peilin Ye <yepeilin.cs@...il.com> wrote:
> On Thu, Jun 01, 2023 at 09:20:39AM +0300, Vlad Buslov wrote:
>> On Wed 31 May 2023 at 20:57, Peilin Ye <yepeilin.cs@...il.com> wrote:
>> > +static inline bool qdisc_is_destroying(const struct Qdisc *qdisc)
>> > +{
>> > +       return qdisc->flags & TCQ_F_DESTROYING;
>> 
>> Hmm, do we need at least some kind of {READ|WRITE}_ONCE() for accessing
>> flags since they are now used in unlocked filter code path?
>
> Thanks, after taking another look at cls_api.c, I noticed this code in
> tc_new_tfilter():
>
> 	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?

>
> 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.