netdev - Re: [PATCH v2 net-next 02/15] neighbour: Move two validations from neigh_get() to neigh_valid_get

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: <CAAVpQUD75jba51sMjWQ=rcG9x7W-uwrpMeVMmTpTVztj8Z9rRA@mail.gmail.com>
Date: Tue, 15 Jul 2025 23:56:48 -0700
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, Simon Horman <horms@...nel.org>, 
	Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 02/15] neighbour: Move two validations from
 neigh_get() to neigh_valid_get_req().

On Tue, Jul 15, 2025 at 6:22 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 12 Jul 2025 20:34:11 +0000 Kuniyuki Iwashima wrote:
> >       err = nlmsg_parse_deprecated_strict(nlh, sizeof(struct ndmsg), tb,
> >                                           NDA_MAX, nda_policy, extack);
> >       if (err < 0)
> > @@ -2951,11 +2957,14 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
> >       }
> >
> >       for (i = 0; i <= NDA_MAX; ++i) {
> > -             if (!tb[i])
> > -                     continue;
> > -
> >               switch (i) {
> >               case NDA_DST:
> > +                     if (!tb[i]) {
> > +                             NL_SET_ERR_MSG(extack, "Network address not specified");
> > +                             err = -EINVAL;
> > +                             goto err;
> > +                     }
>
> Any idea why this attr validation is coded up so weirdly?

Probably the author followed inet_rtm_valid_getroute_req() or
friends that do the similar to be strict and accept only known
attributes.


>
> NDA_DST is 1, so we could make this whole thing:
>
> const struct nla_policy nda_get_policy[] = {
>         [NDA_DST]               = { .type = NLA_BINARY, .len = MAX_ADDR_LEN },
> };
>
>         err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb,
>                           ARRAY_SIZE(nda_get_policy) - 1,
>                           nda_get_policy, extack);
>
> and then no looping over the attributes would be necessary.

Looks cleaner.  I can post a followup series to convert such
places altogether.


>
> I'd also be tempted to replace the extack string with
>         NL_SET_ERR_ATTR_MISS(extack, NULL, NDA_DST);
> but I'm biased towards YNL :)

TIL: NL_SET_ERR_ATTR_MISS, and I like this too so will use it :)