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

Message-ID: <CAM0EoMkw=3SvEDyzyjM3zY60nGTDMdfXYp0Hz43YfVThfmqyTw@mail.gmail.com>
Date: Tue, 4 Mar 2025 11:43:17 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Stanislav Fomichev <stfomichev@...il.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	Cong Wang <xiyou.wangcong@...il.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 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.
IOW, if you are going to respin for another reason then do it for
equivalence sake.
Worst case, we may be able finally find out why the code is that way
when someone reports a strange bug ;->

cheers,
jamal

>
> > Other than that:
> > Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
>
> Thank you for the review!