| Message ID | 20220413105202.2616106-6-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a6cec0bcd34264be8887791594be793b3f12719f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: add flush filtering support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | success | Patch already applied to net-next |
On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote: > Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to > verify that the delete operation allows bulk object deletion. Also emit > a warning if anyone tries to set it for non-delete kind. > > Suggested-by: David Ahern <dsahern@kernel.org> > Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> > --- > v4: new patch > > include/net/rtnetlink.h | 3 ++- > net/core/rtnetlink.c | 8 ++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h > index 0bf622409aaa..bf8bb3357825 100644 > --- a/include/net/rtnetlink.h > +++ b/include/net/rtnetlink.h > @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, > typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *); > > enum rtnl_link_flags { > - RTNL_FLAG_DOIT_UNLOCKED = BIT(0), > + RTNL_FLAG_DOIT_UNLOCKED = BIT(0), > + RTNL_FLAG_BULK_DEL_SUPPORTED = BIT(1), > }; > > enum rtnl_kinds { > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index beda4a7da062..63c7df52a667 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner, > if (dumpit) > link->dumpit = dumpit; > > + WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL && > + (flags & RTNL_FLAG_BULK_DEL_SUPPORTED)); > link->flags |= flags; > > /* publish protocol:msgtype */ > @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, > } > > flags = link->flags; > + if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) && > + !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) { > + NL_SET_ERR_MSG(extack, "Bulk delete is not supported"); > + goto err_unlock; If a buggy user space application is sending messages with NLM_F_BULK set (unintentionally), will it break on newer kernel? I couldn't find where the kernel was validating that reserved flags are not used (I suspect it doesn't). Assuming the above is correct and of interest, maybe just emit a warning via extack and drop the goto? Alternatively, we can see if anyone complains which might never happen > + } > + > if (flags & RTNL_FLAG_DOIT_UNLOCKED) { > doit = link->doit; > rcu_read_unlock(); > -- > 2.35.1 >
On 13/04/2022 15:06, Ido Schimmel wrote: > On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote: >> Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to >> verify that the delete operation allows bulk object deletion. Also emit >> a warning if anyone tries to set it for non-delete kind. >> >> Suggested-by: David Ahern <dsahern@kernel.org> >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >> --- >> v4: new patch >> >> include/net/rtnetlink.h | 3 ++- >> net/core/rtnetlink.c | 8 ++++++++ >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h >> index 0bf622409aaa..bf8bb3357825 100644 >> --- a/include/net/rtnetlink.h >> +++ b/include/net/rtnetlink.h >> @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, >> typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *); >> >> enum rtnl_link_flags { >> - RTNL_FLAG_DOIT_UNLOCKED = BIT(0), >> + RTNL_FLAG_DOIT_UNLOCKED = BIT(0), >> + RTNL_FLAG_BULK_DEL_SUPPORTED = BIT(1), >> }; >> >> enum rtnl_kinds { >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index beda4a7da062..63c7df52a667 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner, >> if (dumpit) >> link->dumpit = dumpit; >> >> + WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL && >> + (flags & RTNL_FLAG_BULK_DEL_SUPPORTED)); >> link->flags |= flags; >> >> /* publish protocol:msgtype */ >> @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, >> } >> >> flags = link->flags; >> + if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) && >> + !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) { >> + NL_SET_ERR_MSG(extack, "Bulk delete is not supported"); >> + goto err_unlock; > > If a buggy user space application is sending messages with NLM_F_BULK > set (unintentionally), will it break on newer kernel? I couldn't find > where the kernel was validating that reserved flags are not used (I > suspect it doesn't). Correct, it doesn't. > > Assuming the above is correct and of interest, maybe just emit a warning > via extack and drop the goto? Alternatively, we can see if anyone > complains which might never happen > TBH I prefer to error out on an unsupported flag, but I get the problem. These weren't validated before and we start checking now. The problem is that we'll return an extack without an error, but the delete might also remove something. Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning in iproute2 terms): "NLM_F_BULK flag is set but bulk delete operation is not supported" So it will warn the user it has an unsupported flag. WDYT ? IMO we should bite the bullet and keep the error though. :) >> + } >> + >> if (flags & RTNL_FLAG_DOIT_UNLOCKED) { >> doit = link->doit; >> rcu_read_unlock(); >> -- >> 2.35.1 >>
On 4/13/22 6:21 AM, Nikolay Aleksandrov wrote: >> If a buggy user space application is sending messages with NLM_F_BULK >> set (unintentionally), will it break on newer kernel? I couldn't find >> where the kernel was validating that reserved flags are not used (I >> suspect it doesn't). > > Correct, it doesn't. > >> >> Assuming the above is correct and of interest, maybe just emit a warning >> via extack and drop the goto? Alternatively, we can see if anyone >> complains which might never happen >> > > TBH I prefer to error out on an unsupported flag, but I get the problem. These > weren't validated before and we start checking now. The problem is that we'll > return an extack without an error, but the delete might also remove something. > Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning > in iproute2 terms): > "NLM_F_BULK flag is set but bulk delete operation is not supported" > So it will warn the user it has an unsupported flag. > > WDYT ? > > IMO we should bite the bullet and keep the error though. :) > I agree. The check across the board for BULK flag on any DELETE requests should tell us pretty quick if someone is setting that flag when it should not be.
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index 0bf622409aaa..bf8bb3357825 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *, typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *); enum rtnl_link_flags { - RTNL_FLAG_DOIT_UNLOCKED = BIT(0), + RTNL_FLAG_DOIT_UNLOCKED = BIT(0), + RTNL_FLAG_BULK_DEL_SUPPORTED = BIT(1), }; enum rtnl_kinds { diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index beda4a7da062..63c7df52a667 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner, if (dumpit) link->dumpit = dumpit; + WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL && + (flags & RTNL_FLAG_BULK_DEL_SUPPORTED)); link->flags |= flags; /* publish protocol:msgtype */ @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, } flags = link->flags; + if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) && + !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) { + NL_SET_ERR_MSG(extack, "Bulk delete is not supported"); + goto err_unlock; + } + if (flags & RTNL_FLAG_DOIT_UNLOCKED) { doit = link->doit; rcu_read_unlock();
Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to verify that the delete operation allows bulk object deletion. Also emit a warning if anyone tries to set it for non-delete kind. Suggested-by: David Ahern <dsahern@kernel.org> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> --- v4: new patch include/net/rtnetlink.h | 3 ++- net/core/rtnetlink.c | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)