| Message ID | 20250217134109.311176-6-idosch@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 34e406a8492832d30491905a878a2ac853e9ef0f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fib_rules: Add port mask support | expand |
On Mon, 17 Feb 2025 15:41:06 +0200 Ido Schimmel <idosch@nvidia.com> wrote: > Allow user space to configure FIB rules that match on the source and > destination ports with a mask, now that support has been added to the > FIB rule core and the IPv4 and IPv6 address families. > > Reviewed-by: Petr Machata <petrm@nvidia.com> > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > --- > net/core/fib_rules.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > index ba6beaa63f44..5ddd34cbe7f6 100644 > --- a/net/core/fib_rules.c > +++ b/net/core/fib_rules.c > @@ -843,8 +843,8 @@ static const struct nla_policy fib_rule_policy[FRA_MAX + > 1] = { [FRA_DSCP] = NLA_POLICY_MAX(NLA_U8, INET_DSCP_MASK >> 2), > [FRA_FLOWLABEL] = { .type = NLA_BE32 }, > [FRA_FLOWLABEL_MASK] = { .type = NLA_BE32 }, > - [FRA_SPORT_MASK] = { .type = NLA_REJECT }, > - [FRA_DPORT_MASK] = { .type = NLA_REJECT }, > + [FRA_SPORT_MASK] = { .type = NLA_U16 }, > + [FRA_DPORT_MASK] = { .type = NLA_U16 }, > }; I don't get the purpose of this patch and patch 1. Couldn't you have patch 3 and 4 first, then patch 2 that adds the netlink and UAPI support?
On Tue, Feb 18, 2025 at 06:15:23PM +0100, Kory Maincent wrote: > On Mon, 17 Feb 2025 15:41:06 +0200 > Ido Schimmel <idosch@nvidia.com> wrote: > > > Allow user space to configure FIB rules that match on the source and > > destination ports with a mask, now that support has been added to the > > FIB rule core and the IPv4 and IPv6 address families. > > > > Reviewed-by: Petr Machata <petrm@nvidia.com> > > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > > --- > > net/core/fib_rules.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > > index ba6beaa63f44..5ddd34cbe7f6 100644 > > --- a/net/core/fib_rules.c > > +++ b/net/core/fib_rules.c > > @@ -843,8 +843,8 @@ static const struct nla_policy fib_rule_policy[FRA_MAX + > > 1] = { [FRA_DSCP] = NLA_POLICY_MAX(NLA_U8, INET_DSCP_MASK >> 2), > > [FRA_FLOWLABEL] = { .type = NLA_BE32 }, > > [FRA_FLOWLABEL_MASK] = { .type = NLA_BE32 }, > > - [FRA_SPORT_MASK] = { .type = NLA_REJECT }, > > - [FRA_DPORT_MASK] = { .type = NLA_REJECT }, > > + [FRA_SPORT_MASK] = { .type = NLA_U16 }, > > + [FRA_DPORT_MASK] = { .type = NLA_U16 }, > > }; > > I don't get the purpose of this patch and patch 1. > Couldn't you have patch 3 and 4 first, then patch 2 that adds the netlink and > UAPI support? Current order is: 1. Add attributes as REJECT. 2. Add support in core. 3. Add support in IPv4. 4. Add support in IPv6. 5. Expose feature to user space. Looks straight forward and easy to review to me and that's the order I prefer. Thanks
On Tue, 18 Feb 2025 20:15:49 +0200 Ido Schimmel <idosch@nvidia.com> wrote: > On Tue, Feb 18, 2025 at 06:15:23PM +0100, Kory Maincent wrote: > > On Mon, 17 Feb 2025 15:41:06 +0200 > > Ido Schimmel <idosch@nvidia.com> wrote: > > > > > Allow user space to configure FIB rules that match on the source and > > > destination ports with a mask, now that support has been added to the > > > FIB rule core and the IPv4 and IPv6 address families. > > > > > > Reviewed-by: Petr Machata <petrm@nvidia.com> > > > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > > > --- > > > net/core/fib_rules.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c > > > index ba6beaa63f44..5ddd34cbe7f6 100644 > > > --- a/net/core/fib_rules.c > > > +++ b/net/core/fib_rules.c > > > @@ -843,8 +843,8 @@ static const struct nla_policy > > > fib_rule_policy[FRA_MAX + 1] = { [FRA_DSCP] = > > > NLA_POLICY_MAX(NLA_U8, INET_DSCP_MASK >> 2), [FRA_FLOWLABEL] = { .type = > > > NLA_BE32 }, [FRA_FLOWLABEL_MASK] = { .type = NLA_BE32 }, > > > - [FRA_SPORT_MASK] = { .type = NLA_REJECT }, > > > - [FRA_DPORT_MASK] = { .type = NLA_REJECT }, > > > + [FRA_SPORT_MASK] = { .type = NLA_U16 }, > > > + [FRA_DPORT_MASK] = { .type = NLA_U16 }, > > > }; > > > > I don't get the purpose of this patch and patch 1. > > Couldn't you have patch 3 and 4 first, then patch 2 that adds the netlink > > and UAPI support? > > Current order is: > > 1. Add attributes as REJECT. > 2. Add support in core. > 3. Add support in IPv4. > 4. Add support in IPv6. > 5. Expose feature to user space. > > Looks straight forward and easy to review to me and that's the order I > prefer. Ok, it is surprising to me. If there is an issue in patch 2,3 or 4. git bisect will locate patch 5 and it won't be easy to find the real patch that cause the issue. Having this type of patch series in the git history will harder the issue debugging. I was not am not a net maintainer so I won't complain more and will let them decide. Regards,
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c index ba6beaa63f44..5ddd34cbe7f6 100644 --- a/net/core/fib_rules.c +++ b/net/core/fib_rules.c @@ -843,8 +843,8 @@ static const struct nla_policy fib_rule_policy[FRA_MAX + 1] = { [FRA_DSCP] = NLA_POLICY_MAX(NLA_U8, INET_DSCP_MASK >> 2), [FRA_FLOWLABEL] = { .type = NLA_BE32 }, [FRA_FLOWLABEL_MASK] = { .type = NLA_BE32 }, - [FRA_SPORT_MASK] = { .type = NLA_REJECT }, - [FRA_DPORT_MASK] = { .type = NLA_REJECT }, + [FRA_SPORT_MASK] = { .type = NLA_U16 }, + [FRA_DPORT_MASK] = { .type = NLA_U16 }, }; int fib_newrule(struct net *net, struct sk_buff *skb, struct nlmsghdr *nlh,