| Message ID | 20220413105202.2616106-5-razor@blackwall.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 545528d788556c724eeb5400757f828ef27782a8 |
| 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 |
Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit : > Add a new delete request modifier called NLM_F_BULK which, when > supported, would cause the request to delete multiple objects. The flag > is a convenient way to signal that a multiple delete operation is > requested which can be gradually added to different delete requests. In > order to make sure older kernels will error out if the operation is not > supported instead of doing something unintended we have to break a > required condition when implementing support for this flag, f.e. for > neighbors we will omit the mandatory mac address attribute. > Initially it will be used to add flush with filtering support for bridge > fdbs, but it also opens the door to add similar support to others. > > Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> > --- > include/uapi/linux/netlink.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h > index 4c0cde075c27..855dffb4c1c3 100644 > --- a/include/uapi/linux/netlink.h > +++ b/include/uapi/linux/netlink.h > @@ -72,6 +72,7 @@ struct nlmsghdr { > > /* Modifiers to DELETE request */ > #define NLM_F_NONREC 0x100 /* Do not delete recursively */ > +#define NLM_F_BULK 0x200 /* Delete multiple objects */ Sorry to reply to an old patch, but FWIW, this patch broke the uAPI. One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is conceptually wrong but it was working. After this patch, the kernel returns an error (EOPNOTSUPP). Here is the patch series: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b We probably can't do anything now, but to avoid this in the future, I see only two options: - enforce flags validation depending on the operation (but this may break some existing apps) - stop adding new flags that overlap between NEW and DEL operations (by adding a comment or defining dummy flags). Any thoughts? Regards, Nicolas
On 20/09/2022 10:49, Nicolas Dichtel wrote: > > Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit : >> Add a new delete request modifier called NLM_F_BULK which, when >> supported, would cause the request to delete multiple objects. The flag >> is a convenient way to signal that a multiple delete operation is >> requested which can be gradually added to different delete requests. In >> order to make sure older kernels will error out if the operation is not >> supported instead of doing something unintended we have to break a >> required condition when implementing support for this flag, f.e. for >> neighbors we will omit the mandatory mac address attribute. >> Initially it will be used to add flush with filtering support for bridge >> fdbs, but it also opens the door to add similar support to others. >> >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >> --- >> include/uapi/linux/netlink.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h >> index 4c0cde075c27..855dffb4c1c3 100644 >> --- a/include/uapi/linux/netlink.h >> +++ b/include/uapi/linux/netlink.h >> @@ -72,6 +72,7 @@ struct nlmsghdr { >> >> /* Modifiers to DELETE request */ >> #define NLM_F_NONREC 0x100 /* Do not delete recursively */ >> +#define NLM_F_BULK 0x200 /* Delete multiple objects */ > Sorry to reply to an old patch, but FWIW, this patch broke the uAPI. > One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is > conceptually wrong but it was working. After this patch, the kernel returns an > error (EOPNOTSUPP). > > Here is the patch series: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b > > We probably can't do anything now, but to avoid this in the future, I see only > two options: > - enforce flags validation depending on the operation (but this may break some > existing apps) > - stop adding new flags that overlap between NEW and DEL operations (by adding > a comment or defining dummy flags). > > Any thoughts? > Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space applications, but we can break someone (who arguably should fix their app though). We already had that discussion while the set was under review[1] and just to be a bit more confident I also tried searching for open-source buggy users, but didn't find any. > Regards, > Nicolas [1] https://lore.kernel.org/netdev/97774474-65a3-fa45-e0b9-8db6c748da28@kernel.org/t/#m23018ce831dae16d42cb9c393c7c6bad1bc621c3
Le 20/09/2022 à 11:05, Nikolay Aleksandrov a écrit : > On 20/09/2022 10:49, Nicolas Dichtel wrote: >> >> Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit : >>> Add a new delete request modifier called NLM_F_BULK which, when >>> supported, would cause the request to delete multiple objects. The flag >>> is a convenient way to signal that a multiple delete operation is >>> requested which can be gradually added to different delete requests. In >>> order to make sure older kernels will error out if the operation is not >>> supported instead of doing something unintended we have to break a >>> required condition when implementing support for this flag, f.e. for >>> neighbors we will omit the mandatory mac address attribute. >>> Initially it will be used to add flush with filtering support for bridge >>> fdbs, but it also opens the door to add similar support to others. >>> >>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> >>> --- >>> include/uapi/linux/netlink.h | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h >>> index 4c0cde075c27..855dffb4c1c3 100644 >>> --- a/include/uapi/linux/netlink.h >>> +++ b/include/uapi/linux/netlink.h >>> @@ -72,6 +72,7 @@ struct nlmsghdr { >>> >>> /* Modifiers to DELETE request */ >>> #define NLM_F_NONREC 0x100 /* Do not delete recursively */ >>> +#define NLM_F_BULK 0x200 /* Delete multiple objects */ >> Sorry to reply to an old patch, but FWIW, this patch broke the uAPI. >> One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is >> conceptually wrong but it was working. After this patch, the kernel returns an >> error (EOPNOTSUPP). >> >> Here is the patch series: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b >> >> We probably can't do anything now, but to avoid this in the future, I see only >> two options: >> - enforce flags validation depending on the operation (but this may break some >> existing apps) >> - stop adding new flags that overlap between NEW and DEL operations (by adding >> a comment or defining dummy flags). >> >> Any thoughts? >> > > Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space > applications, but we can break someone (who arguably should fix their app though). We already had > that discussion while the set was under review[1] and just to be a bit more confident I also Thanks for the link. Finally, someone has (almost) complained :D > tried searching for open-source buggy users, but didn't find any. The trend seems to let someone else add another specific flag if needed. Thus, it seems that checking flags is the way to go. The pro is that if someone complains, the patch could be reverted, which is not the case for a new feature like this bulk for example. Regards, Nicolas
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h index 4c0cde075c27..855dffb4c1c3 100644 --- a/include/uapi/linux/netlink.h +++ b/include/uapi/linux/netlink.h @@ -72,6 +72,7 @@ struct nlmsghdr { /* Modifiers to DELETE request */ #define NLM_F_NONREC 0x100 /* Do not delete recursively */ +#define NLM_F_BULK 0x200 /* Delete multiple objects */ /* Flags for ACK message */ #define NLM_F_CAPPED 0x100 /* request was capped */
Add a new delete request modifier called NLM_F_BULK which, when supported, would cause the request to delete multiple objects. The flag is a convenient way to signal that a multiple delete operation is requested which can be gradually added to different delete requests. In order to make sure older kernels will error out if the operation is not supported instead of doing something unintended we have to break a required condition when implementing support for this flag, f.e. for neighbors we will omit the mandatory mac address attribute. Initially it will be used to add flush with filtering support for bridge fdbs, but it also opens the door to add similar support to others. Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org> --- include/uapi/linux/netlink.h | 1 + 1 file changed, 1 insertion(+)