netdev - Re: [PATCH net-next v10 03/14] net: sched: wrap doit/dumpit methods

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: <Z8doZoKNLQQYXYuf@pop-os.localdomain>
Date: Tue, 4 Mar 2025 12:53:58 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Stanislav Fomichev <stfomichev@...il.com>,
	Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, Jiri Pirko <jiri@...nulli.us>,
	Saeed Mahameed <saeed@...nel.org>
Subject: Re: [PATCH net-next v10 03/14] net: sched: wrap doit/dumpit methods

On Tue, Mar 04, 2025 at 11:43:17AM -0500, Jamal Hadi Salim wrote:
> On Mon, Mar 3, 2025 at 10:33 AM Stanislav Fomichev <stfomichev@...il.com> wrote:
> >
> > On 03/02, Jamal Hadi Salim wrote:
> > > On Sat, Mar 1, 2025 at 7:09 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
> > > >
> > > > In preparation for grabbing netdev instance lock around qdisc
> > > > operations, introduce tc_xxx wrappers that lookup netdev
> > > > and call respective __tc_xxx helper to do the actual work.
> > > > No functional changes.
> > > >
> > > > Cc: Jamal Hadi Salim <jhs@...atatu.com>
> > > > Cc: Cong Wang <xiyou.wangcong@...il.com>
> > > > Cc: Jiri Pirko <jiri@...nulli.us>
> > > > Cc: Saeed Mahameed <saeed@...nel.org>
> > > > Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> > > > ---
> > > >  net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
> > > >  1 file changed, 122 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > > index e3e91cf867eb..e0be3af4daa9 100644
> > > > --- a/net/sched/sch_api.c
> > > > +++ b/net/sched/sch_api.c
> > > > @@ -1505,27 +1505,18 @@ const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> > > >   * Delete/get qdisc.
> > > >   */
> > > >
> > > [..]
> > > > +static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> > > > +                          struct netlink_ext_ack *extack)
> > > > +{
> > > > +       struct net *net = sock_net(skb->sk);
> > > > +       struct tcmsg *tcm = nlmsg_data(n);
> > > > +       struct nlattr *tca[TCA_MAX + 1];
> > > > +       struct net_device *dev;
> > > > +       bool replay;
> > > > +       int err;
> > > > +
> > > > +replay:
> > >
> > > For 1-1 mapping to original code, the line:
> > > struct tcmsg *tcm = nlmsg_data(n);
> > >
> > > Should move below the replay goto..
> >
> > Since nlmsg_data is just doing pointer arithmetics and the (pointer) argument
> > doesn't change I was assuming it's ok to reshuffle to make
> > tc_get_qdisc and tc_modify_qdisc look more consistent. LMK if you
> > disagree, happy to move them back.
> >
> 
> TBH, I never understood why we had to reinit for the netlink messaging
> per that comment: /* Reinit, just in case something touches this. */
> I dont think anything will change that netlink message, but who knows
> there could be some niche case.

As long as all TDC tests pass, we can keep the patch as it is. Now you
can see why I have been pushing people to add TDC tests, they are a
warranty of safety for code refactoring like this one. :)

Thanks.