Re: [PATCH net-next v8 01/24] netlink: add NLA_POLICY_MAX_LEN macro

[Antonio Quartulli]

Hi,

On 04/10/2024 15:38, Jakub Kicinski wrote:
> On Fri, 04 Oct 2024 13:58:04 +0100 Donald Hunter wrote:
> > > @@ -466,6 +466,8 @@ class TypeBinary(Type):
> > >       def _attr_policy(self, policy):
> > >           if 'exact-len' in self.checks:
> > >               mem = 'NLA_POLICY_EXACT_LEN(' + str(self.get_limit('exact-len')) + ')'
> > > +        elif 'max-len' in self.checks:
> > > +            mem = 'NLA_POLICY_MAX_LEN(' + str(self.get_limit('max-len')) + ')'
> >
> > This takes precedence over min-length. What if both are set? The logic
> > should probably check and use NLA_POLICY_RANGE
>
> Or we could check if len(self.checks) <= 1 early and throw our hands up
> if there is more, for now?

We already perform the same check in the 'else' branch below.
It'd be about moving it at the beginning of the function and bail out if 
true, right?

Should I modify this patch and move the check above?


Cheers,

> > >           else:
> > >               mem = '{ '
> > >               if len(self.checks) == 1 and 'min-len' in self.checks:
> >
> > Perhaps this should use NLA_POLICY_MIN_LEN ? In fact the current code
> > looks broken to me because the NLA_BINARY len check in validate_nla() is
> > a max length check, right?
> >
> > https://elixir.bootlin.com/linux/v6.11.1/source/lib/nlattr.c#L499
> >
> > The alternative is you emit an explicit initializer that includes the
> > correct NLA_VALIDATE_* type and sets type, min and/or max.
>
> Yeah, this code leads to endless confusion. We use NLA_UNSPEC (0)
> if min-len is set (IOW we don't set .type to NLA_BINARY). NLA_UNSPEC
> has different semantics for len.
>
> Agreed that we should probably clean this up, but no bug AFAICT.

--
Antonio Quartulli
OpenVPN Inc.