Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On 10/2/18 10:30 AM, Jiri Benc wrote: > On Tue, 2 Oct 2018 08:57:24 -0600, David Ahern wrote: >> You can when you introduce a new option or a new flag that is required >> to get new behavior like kernel side filtering. > > Yes. That was what I tried with the patchset a few years back. It would > be nice to revive the effort. > >> I chose a netlink flag for consistency with NLM_F_DUMP_INTR and >> NLM_F_DUMP_FILTERED. Both are netlink flags. This patch set fixes only >> what is broken -- dumps. > > When we're introducing better input checking in netlink (which is a > good thing!), it would be good to do it consistently and have it > generic across all operations. > Thinking about this some more... a setsockopt to enable the new checks would provide a definitive way for userspace to know if the feature is supported or not. As for expanding the scope to NEW (and maybe DEL) commands (which was the point of your patch set 3 years ago), I think is an idealistic goal that is not practical to implement. Trying to go through all commands and cover all options and valid combinations to report back errors is a herculean effort with little return on the time investment. New commands and new features should certainly add rigid checks for valid combinations, but a retrospective audit on existing commands is a time sink. Perhaps new attributes can be checked (new command on old kernel); seems like that can be covered by a simple change to nla_parse to handle type > maxtype based on a new input flag. Once the flag goes in, all of the commands to be affected by it have to be done in the same release. I have done that for the dump commands which is fairly easy considering the existing code.
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On Tue, 2 Oct 2018 08:55:19 -0600, David Ahern wrote: > You want take issue with a name suggest a different one. I don't have a strong opinion on this. STRICT_HDR? HDR_CHECKED? VERIFY_HDR? If you feel that your proposal is better, I don't mind. Thanks, Jiri
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On Tue, 2 Oct 2018 08:57:24 -0600, David Ahern wrote: > You can when you introduce a new option or a new flag that is required > to get new behavior like kernel side filtering. Yes. That was what I tried with the patchset a few years back. It would be nice to revive the effort. > I chose a netlink flag for consistency with NLM_F_DUMP_INTR and > NLM_F_DUMP_FILTERED. Both are netlink flags. This patch set fixes only > what is broken -- dumps. When we're introducing better input checking in netlink (which is a good thing!), it would be good to do it consistently and have it generic across all operations. Jiri
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On 10/2/18 5:27 AM, Jiri Benc wrote: > On Tue, 2 Oct 2018 13:18:32 +0200, Christian Brauner wrote: >> I didn't find this in the linked thread. > > Maybe it was suggested in another thread or in person on a conference, > I can't remember, it's too long ago, sorry. > >> What I find interesting and convincing is one of Dave's points: >> >> "I'm beginning to wonder if we can just change this unilaterally to >> not ignore unrecognized attributes. >> >> I am increasingly certain that things that would "break" we wouldn't >> want to succeed anyways." [1] > > It's unfortunate we can't do that. I'd like it. You can when you introduce a new option or a new flag that is required to get new behavior like kernel side filtering. > >> But a socket option or this header flag both sound acceptable to me. Was >> there any more detail on how a socket option would look like, i.e. an >> api proposal or something? > > Look at how NETLINK_CAP_ACK and NETLINK_EXT_ACK is implemented. I chose a netlink flag for consistency with NLM_F_DUMP_INTR and NLM_F_DUMP_FILTERED. Both are netlink flags. This patch set fixes only what is broken -- dumps.
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On 10/2/18 5:06 AM, Jiri Benc wrote: > On Mon, 1 Oct 2018 17:28:29 -0700, David Ahern wrote: >> Add a new flag, NLM_F_DUMP_PROPER_HDR, for userspace to indicate to the >> kernel that it believes it is sending the right header struct for the >> dump message type (ifinfomsg, ifaddrmsg, rtmsg, fib_rule_hdr, ...). > > Why is this limited to dumps? Other kind of netlink messages contain > the common struct, too. When introducing such mechanism, please make it > generic. Because all of the other requests -- NEW, DEL, and GET -- all seem to use and expect the right header. Dumps are the ones where the header is not looked at in most cases and for years iproute2 got away with sending the wrong one. > > Last time when we were discussing strict checking in netlink, it was > suggested to add a socket option instead of adding NLM flags[1]. > It makes a lot of sense: the number of flags is very limited and we'd > run out of them pretty fast. It's not just the header structure that > is currently checked sloppily. It's also attributes, flags in > attributes, etc. We can't assign a flag to all of them. > > You should also consider a different name for the flag: it should > reflect what the effect of the flag is. "Proper header" is not an > effect, it's a requirement for the message to pass. The effect is > enforced strict checking of the header. Proper means the correct header for the dump type is sent. You want take issue with a name suggest a different one.
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On Tue, 2 Oct 2018 13:18:32 +0200, Christian Brauner wrote: > I didn't find this in the linked thread. Maybe it was suggested in another thread or in person on a conference, I can't remember, it's too long ago, sorry. > What I find interesting and convincing is one of Dave's points: > > "I'm beginning to wonder if we can just change this unilaterally to > not ignore unrecognized attributes. > > I am increasingly certain that things that would "break" we wouldn't > want to succeed anyways." [1] It's unfortunate we can't do that. I'd like it. > But a socket option or this header flag both sound acceptable to me. Was > there any more detail on how a socket option would look like, i.e. an > api proposal or something? Look at how NETLINK_CAP_ACK and NETLINK_EXT_ACK is implemented. Jiri
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On Tue, Oct 02, 2018 at 01:06:14PM +0200, Jiri Benc wrote: > On Mon, 1 Oct 2018 17:28:29 -0700, David Ahern wrote: > > Add a new flag, NLM_F_DUMP_PROPER_HDR, for userspace to indicate to the > > kernel that it believes it is sending the right header struct for the > > dump message type (ifinfomsg, ifaddrmsg, rtmsg, fib_rule_hdr, ...). > > Why is this limited to dumps? Other kind of netlink messages contain > the common struct, too. When introducing such mechanism, please make it > generic. > > Last time when we were discussing strict checking in netlink, it was > suggested to add a socket option instead of adding NLM flags[1]. I didn't find this in the linked thread. What I find interesting and convincing is one of Dave's points: "I'm beginning to wonder if we can just change this unilaterally to not ignore unrecognized attributes. I am increasingly certain that things that would "break" we wouldn't want to succeed anyways." [1] :) But a socket option or this header flag both sound acceptable to me. Was there any more detail on how a socket option would look like, i.e. an api proposal or something? [1]: https://marc.info/?l=linux-netdev=144522081220166=2 > It makes a lot of sense: the number of flags is very limited and we'd > run out of them pretty fast. It's not just the header structure that > is currently checked sloppily. It's also attributes, flags in > attributes, etc. We can't assign a flag to all of them. > > You should also consider a different name for the flag: it should > reflect what the effect of the flag is. "Proper header" is not an > effect, it's a requirement for the message to pass. The effect is > enforced strict checking of the header. > > Jiri > > [1] https://marc.info/?l=linux-netdev=144492718118955
Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
On Mon, 1 Oct 2018 17:28:29 -0700, David Ahern wrote: > Add a new flag, NLM_F_DUMP_PROPER_HDR, for userspace to indicate to the > kernel that it believes it is sending the right header struct for the > dump message type (ifinfomsg, ifaddrmsg, rtmsg, fib_rule_hdr, ...). Why is this limited to dumps? Other kind of netlink messages contain the common struct, too. When introducing such mechanism, please make it generic. Last time when we were discussing strict checking in netlink, it was suggested to add a socket option instead of adding NLM flags[1]. It makes a lot of sense: the number of flags is very limited and we'd run out of them pretty fast. It's not just the header structure that is currently checked sloppily. It's also attributes, flags in attributes, etc. We can't assign a flag to all of them. You should also consider a different name for the flag: it should reflect what the effect of the flag is. "Proper header" is not an effect, it's a requirement for the message to pass. The effect is enforced strict checking of the header. Jiri [1] https://marc.info/?l=linux-netdev=144492718118955
[PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
From: David Ahern
Add a new flag, NLM_F_DUMP_PROPER_HDR, for userspace to indicate to the
kernel that it believes it is sending the right header struct for the
dump message type (ifinfomsg, ifaddrmsg, rtmsg, fib_rule_hdr, ...).
Setting the flag in the netlink message header indicates to the kernel
it should do rigid checking on all data passed in the dump request and
filter the data returned based on data passed in.
Signed-off-by: David Ahern
---
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 776bc92e9118..e722bed88dee 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -57,6 +57,7 @@ struct nlmsghdr {
#define NLM_F_ECHO 0x08/* Echo this request*/
#define NLM_F_DUMP_INTR0x10/* Dump was inconsistent due to
sequence change */
#define NLM_F_DUMP_FILTERED0x20/* Dump was filtered as requested */
+#define NLM_F_DUMP_PROPER_HDR 0x40/* Dump request has the proper header
for type */
/* Modifiers to GET request */
#define NLM_F_ROOT 0x100 /* specify tree root*/
--
2.11.0
