| Message ID | 20211111160240.739294-2-alexander.mikhalitsyn@virtuozzo.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] rtnetlink: add RTNH_F_REJECT_MASK | expand |
On Thu, 11 Nov 2021 19:02:40 +0300 Alexander Mikhalitsyn wrote: > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index 5888492a5257..c15e591e5d25 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -417,6 +417,9 @@ struct rtnexthop { > #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ > RTNH_F_OFFLOAD | RTNH_F_TRAP) > > +/* these flags can't be set by the userspace */ > +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) You should probably drop the _F_ since RTNH_COMPARE_MASK above does not have it.
Dear Jakub, Thanks for your attention to the patch. Sure, I will do it. Please, let me know, what do you think about RTNH_F_OFFLOAD, RTNH_F_TRAP flags? Don't we need to prohibit it too? Alex On Thu, Nov 11, 2021 at 8:48 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 11 Nov 2021 19:02:40 +0300 Alexander Mikhalitsyn wrote: > > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > > index 5888492a5257..c15e591e5d25 100644 > > --- a/include/uapi/linux/rtnetlink.h > > +++ b/include/uapi/linux/rtnetlink.h > > @@ -417,6 +417,9 @@ struct rtnexthop { > > #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ > > RTNH_F_OFFLOAD | RTNH_F_TRAP) > > > > +/* these flags can't be set by the userspace */ > > +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) > > You should probably drop the _F_ since RTNH_COMPARE_MASK above > does not have it.
On Thu, 11 Nov 2021 20:51:20 +0300 Alexander Mikhalitsyn wrote: > Thanks for your attention to the patch. Sure, I will do it. > > Please, let me know, what do you think about RTNH_F_OFFLOAD, > RTNH_F_TRAP flags? Don't we need to prohibit it too? Looks like an omission indeed but I'll let Dave and Ido comment. Reminder: please don't top post on the ML.
On Thu, Nov 11, 2021 at 8:56 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 11 Nov 2021 20:51:20 +0300 Alexander Mikhalitsyn wrote: > > Thanks for your attention to the patch. Sure, I will do it. > > > > Please, let me know, what do you think about RTNH_F_OFFLOAD, > > RTNH_F_TRAP flags? Don't we need to prohibit it too? > > Looks like an omission indeed but I'll let Dave and Ido comment. Sure. > > Reminder: please don't top post on the ML. Oh, yep. I'm sorry.
On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote: > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index 5888492a5257..c15e591e5d25 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -417,6 +417,9 @@ struct rtnexthop { > #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ > RTNH_F_OFFLOAD | RTNH_F_TRAP) > > +/* these flags can't be set by the userspace */ > +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) > + > /* Macros to handle hexthops */ Userspace can not set any of the flags in RTNH_COMPARE_MASK.
On Thu, Nov 11, 2021 at 10:13 PM David Ahern <dsahern@gmail.com> wrote: > > On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote: > > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > > index 5888492a5257..c15e591e5d25 100644 > > --- a/include/uapi/linux/rtnetlink.h > > +++ b/include/uapi/linux/rtnetlink.h > > @@ -417,6 +417,9 @@ struct rtnexthop { > > #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ > > RTNH_F_OFFLOAD | RTNH_F_TRAP) > > > > +/* these flags can't be set by the userspace */ > > +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) > > + > > /* Macros to handle hexthops */ > > Userspace can not set any of the flags in RTNH_COMPARE_MASK. Hi David, thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags against RTNH_COMPARE_MASK. So, there is no need to introduce a separate RTNH_F_REJECT_MASK. Am I right? Regards, Alex
[ cc roopa] On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote: > On Thu, Nov 11, 2021 at 10:13 PM David Ahern <dsahern@gmail.com> wrote: >> >> On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote: >>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >>> index 5888492a5257..c15e591e5d25 100644 >>> --- a/include/uapi/linux/rtnetlink.h >>> +++ b/include/uapi/linux/rtnetlink.h >>> @@ -417,6 +417,9 @@ struct rtnexthop { >>> #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ >>> RTNH_F_OFFLOAD | RTNH_F_TRAP) >>> >>> +/* these flags can't be set by the userspace */ >>> +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) >>> + >>> /* Macros to handle hexthops */ >> >> Userspace can not set any of the flags in RTNH_COMPARE_MASK. > > Hi David, > > thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags > against RTNH_COMPARE_MASK. So, there is no need to introduce a separate > RTNH_F_REJECT_MASK. > Am I right? > Added Roopa to double check if Cumulus relies on this for their switchd. If that answer is no, then there is no need for a new mask.
On 11/11/21 2:19 PM, David Ahern wrote: > [ cc roopa] > > On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote: >> On Thu, Nov 11, 2021 at 10:13 PM David Ahern <dsahern@gmail.com> wrote: >>> On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote: >>>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >>>> index 5888492a5257..c15e591e5d25 100644 >>>> --- a/include/uapi/linux/rtnetlink.h >>>> +++ b/include/uapi/linux/rtnetlink.h >>>> @@ -417,6 +417,9 @@ struct rtnexthop { >>>> #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ >>>> RTNH_F_OFFLOAD | RTNH_F_TRAP) >>>> >>>> +/* these flags can't be set by the userspace */ >>>> +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) >>>> + >>>> /* Macros to handle hexthops */ >>> Userspace can not set any of the flags in RTNH_COMPARE_MASK. >> Hi David, >> >> thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags >> against RTNH_COMPARE_MASK. So, there is no need to introduce a separate >> RTNH_F_REJECT_MASK. >> Am I right? >> > Added Roopa to double check if Cumulus relies on this for their switchd. > > If that answer is no, then there is no need for a new mask. > yes, these flags are already exposed to userspace and we do use it. We have also considered optimizations where routing daemons set OFFLOAD and drivers clear it when offload fails. I wont be surprised if other open network os distributions are also using it. Thanks for the headsup David.
On 11/11/21 6:02 PM, Roopa Prabhu wrote: > > On 11/11/21 2:19 PM, David Ahern wrote: >> [ cc roopa] >> >> On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote: >>> On Thu, Nov 11, 2021 at 10:13 PM David Ahern <dsahern@gmail.com> wrote: >>>> On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote: >>>>> diff --git a/include/uapi/linux/rtnetlink.h >>>>> b/include/uapi/linux/rtnetlink.h >>>>> index 5888492a5257..c15e591e5d25 100644 >>>>> --- a/include/uapi/linux/rtnetlink.h >>>>> +++ b/include/uapi/linux/rtnetlink.h >>>>> @@ -417,6 +417,9 @@ struct rtnexthop { >>>>> #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ >>>>> RTNH_F_OFFLOAD | RTNH_F_TRAP) >>>>> >>>>> +/* these flags can't be set by the userspace */ >>>>> +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) >>>>> + >>>>> /* Macros to handle hexthops */ >>>> Userspace can not set any of the flags in RTNH_COMPARE_MASK. >>> Hi David, >>> >>> thanks! So, I have to prepare a patch which fixes current checks for >>> rtnh_flags >>> against RTNH_COMPARE_MASK. So, there is no need to introduce a separate >>> RTNH_F_REJECT_MASK. >>> Am I right? >>> >> Added Roopa to double check if Cumulus relies on this for their switchd. >> >> If that answer is no, then there is no need for a new mask. >> > > yes, these flags are already exposed to userspace and we do use it. > > We have also considered optimizations where routing daemons set OFFLOAD > and drivers clear it when offload fails. > > I wont be surprised if other open network os distributions are also > using it. > > > Thanks for the headsup David. > Thanks, Roopa. So then the separate mask is needed.
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 5888492a5257..c15e591e5d25 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -417,6 +417,9 @@ struct rtnexthop { #define RTNH_COMPARE_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN | \ RTNH_F_OFFLOAD | RTNH_F_TRAP) +/* these flags can't be set by the userspace */ +#define RTNH_F_REJECT_MASK (RTNH_F_DEAD | RTNH_F_LINKDOWN) + /* Macros to handle hexthops */ #define RTNH_ALIGNTO 4 diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index 4c0c33e4710d..7a383c54fe46 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh, return -EINVAL; } - if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) { + if (rtnh->rtnh_flags & RTNH_F_REJECT_MASK) { NL_SET_ERR_MSG(extack, "Invalid flags for nexthop - can not contain DEAD or LINKDOWN"); return -EINVAL; @@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg, goto err_inval; } - if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) { + if (cfg->fc_flags & RTNH_F_REJECT_MASK) { NL_SET_ERR_MSG(extack, "Invalid rtm_flags - can not contain DEAD or LINKDOWN"); goto err_inval;
Introduce RTNH_F_REJECT_MASK mask which contains all rtnh_flags which can't be set by the userspace directly. This mask will be used in the iproute utility to exclude rtnh_flags which can't be restored from "ip route save" image. This patch doesn't change kernel behavior, but it looks like we need to prohibit setting RTNH_F_OFFLOAD, RTNH_F_TRAP flags too. Am I right? Please, take a look on [RFC PATCH iproute2] ip route: save: exclude rtnh_flags which can't be set Cc: David Miller <davem@davemloft.net> Cc: David Ahern <dsahern@gmail.com> Cc: Stephen Hemminger <stephen@networkplumber.org> Cc: Ido Schimmel <idosch@nvidia.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Andrei Vagin <avagin@gmail.com> Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com> --- include/uapi/linux/rtnetlink.h | 3 +++ net/ipv4/fib_semantics.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-)