netdev - Re: [PATCH net] net: sched: refine software bypass handling in tc

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: <CADvbK_cBD_JW5_x0HWY7f4uM7cgyfCvzBSyu_EL=XX7m7VJwhw@mail.gmail.com>
Date: Mon, 13 Jan 2025 10:19:01 -0500
From: Xin Long <lucien.xin@...il.com>
To: Asbjørn Sloth Tønnesen <ast@...erby.net>
Cc: davem@...emloft.net, kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Jamal Hadi Salim <jhs@...atatu.com>, 
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, Shuang Li <shuali@...hat.com>, 
	network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: sched: refine software bypass handling in tc_run

On Fri, Jan 10, 2025 at 8:25 AM Asbjørn Sloth Tønnesen <ast@...erby.net> wrote:
>
> Sorry for the late response, it has been a busy first week back, too many
> operational issues with devices in our network running crappy vendor images.
>
> On 1/6/25 3:08 PM, Xin Long wrote:
> > This patch addresses issues with filter counting in block (tcf_block),
> > particularly for software bypass scenarios, by introducing a more
> > accurate mechanism using useswcnt.
> >
> > Previously, filtercnt and skipswcnt were introduced by:
> >
> >    Commit 2081fd3445fe ("net: sched: cls_api: add filter counter") and
> >    Commit f631ef39d819 ("net: sched: cls_api: add skip_sw counter")
> >
> >    filtercnt tracked all tp (tcf_proto) objects added to a block, and
> >    skipswcnt counted tp objects with the skipsw attribute set.
> >
> > The problem is: a single tp can contain multiple filters, some with skipsw
> > and others without. The current implementation fails in the case:
> >
> >    When the first filter in a tp has skipsw, both skipswcnt and filtercnt
> >    are incremented, then adding a second filter without skipsw to the same
> >    tp does not modify these counters because tp->counted is already set.
> >
> >    This results in bypass software behavior based solely on skipswcnt
> >    equaling filtercnt, even when the block includes filters without
> >    skipsw. Consequently, filters without skipsw are inadvertently bypassed.
>
> Thank you for tracking it down. I wasn't aware that a tp, could be used by multiple
> filters, and didn't encounter it during my testing.
>
> > To address this, the patch introduces useswcnt in block to explicitly count
> > tp objects containing at least one filter without skipsw. Key changes
> > include:
> >
> >    Whenever a filter without skipsw is added, its tp is marked with usesw
> >    and counted in useswcnt. tc_run() now uses useswcnt to determine software
> >    bypass, eliminating reliance on filtercnt and skipswcnt.
> >
> >    This refined approach prevents software bypass for blocks containing
> >    mixed filters, ensuring correct behavior in tc_run().
> >
> > Additionally, as atomic operations on useswcnt ensure thread safety and
> > tp->lock guards access to tp->usesw and tp->counted, the broader lock
> > down_write(&block->cb_lock) is no longer required in tc_new_tfilter(),
> > and this resolves a performance regression caused by the filter counting
> > mechanism during parallel filter insertions.
>
> You are trying to do two things:
> A) Fix functional defect when filters share a single tp
> B) Improve filter updates performance
>
> If you do part A in a minimalistic way, then IMHO it might be suitable
> for net (+ stable), but for part B I agree with Paolo, that it would
> properly be better suited for net-next.
>
> I focused my testing on routing performance, not filter update performance,
> I also didn't test it in any multi-CPU setups (as I don't have any).
>
> The static key was added to mitigate concerns, about the impact that the
> bypass check would have for non-offloaded workloads in multi-CPU systems.
>
> https://lore.kernel.org/netdev/28bf1467-b7ce-4e36-a4ef-5445f65edd97@fiberby.net/
> https://lore.kernel.org/netdev/CAM0EoMngVoBcbX7cqTdbW8dG1v_ysc1SZK+4y-9j-5Tbq6gaYw@mail.gmail.com/
Hi, Asbjørn, thanks for the comment.

I will keep the static key, and not touch the code in tc_run() in
net/core/dev.c, and we can make it without holding the block->cb_lock
by atomic_inc/dec_return():

        if (atomic_inc_return(&block->useswcnt) == 1)
                static_branch_inc(&tcf_bypass_check_needed_key);

        if (!atomic_dec_return(&tp->chain->block->useswcnt))
                static_branch_dec(&tcf_bypass_check_needed_key);

It doesn't look good to split this patch into two, I will post v2 on
net.git with these changes.

Let me know if there are any other concerns.