| Message ID | 20241002-b4-ovpn-v8-1-37ceffcffbde@openvpn.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
Antonio Quartulli <antonio@openvpn.net> writes: > Similarly to NLA_POLICY_MIN_LEN, NLA_POLICY_MAX_LEN defines a policy > with a maximum length value. > > The netlink generator for YAML specs has been extended accordingly. > > Cc: donald.hunter@gmail.com > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > include/net/netlink.h | 1 + > tools/net/ynl/ynl-gen-c.py | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/include/net/netlink.h b/include/net/netlink.h > index db6af207287c839408c58cb28b82408e0548eaca..2dc671c977ff3297975269d236264907009703d3 100644 > --- a/include/net/netlink.h > +++ b/include/net/netlink.h > @@ -469,6 +469,7 @@ struct nla_policy { > .max = _len \ > } > #define NLA_POLICY_MIN_LEN(_len) NLA_POLICY_MIN(NLA_BINARY, _len) > +#define NLA_POLICY_MAX_LEN(_len) NLA_POLICY_MAX(NLA_BINARY, _len) > > /** > * struct nl_info - netlink source information > diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py > index 717530bc9c52e7cfa897814870b4583c88618a27..3ccbb301be87f80bbcf03da63d60f58c4fedc1c8 100755 > --- a/tools/net/ynl/ynl-gen-c.py > +++ b/tools/net/ynl/ynl-gen-c.py > @@ -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 > 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.
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? > > 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.
On Fri, 4 Oct 2024 at 14:38, Jakub Kicinski <kuba@kernel.org> 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? > > > > 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. Oh, I see it now. So it's dropping through to here: https://elixir.bootlin.com/linux/v6.11.1/source/lib/nlattr.c#L555 > Agreed that we should probably clean this up, but no bug AFAICT. Yeah, it's definitely surprising that the meaning of .len varies.
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.
On Mon, 7 Oct 2024 12:04:22 +0200 Antonio Quartulli wrote: > > 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? I just sent the refactor patch, that seemed easier than explaining ;)
On 07/10/24 17:53, Jakub Kicinski wrote: > On Mon, 7 Oct 2024 12:04:22 +0200 Antonio Quartulli wrote: >>> 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? > > I just sent the refactor patch, that seemed easier than explaining ;) Great, thanks :-)
diff --git a/include/net/netlink.h b/include/net/netlink.h index db6af207287c839408c58cb28b82408e0548eaca..2dc671c977ff3297975269d236264907009703d3 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -469,6 +469,7 @@ struct nla_policy { .max = _len \ } #define NLA_POLICY_MIN_LEN(_len) NLA_POLICY_MIN(NLA_BINARY, _len) +#define NLA_POLICY_MAX_LEN(_len) NLA_POLICY_MAX(NLA_BINARY, _len) /** * struct nl_info - netlink source information diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index 717530bc9c52e7cfa897814870b4583c88618a27..3ccbb301be87f80bbcf03da63d60f58c4fedc1c8 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -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')) + ')' else: mem = '{ ' if len(self.checks) == 1 and 'min-len' in self.checks:
Similarly to NLA_POLICY_MIN_LEN, NLA_POLICY_MAX_LEN defines a policy with a maximum length value. The netlink generator for YAML specs has been extended accordingly. Cc: donald.hunter@gmail.com Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- include/net/netlink.h | 1 + tools/net/ynl/ynl-gen-c.py | 2 ++ 2 files changed, 3 insertions(+)