| Message ID | 58865c4c143d0da40cd417b5b87b49d292d8129d.1560561432.git.sbrivio@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Fix listing (IPv4, IPv6) and flushing (IPv6) of cached route exceptions | expand |
On 6/14/19 7:32 PM, Stefano Brivio wrote: > Socket option NETLINK_GET_STRICT_CHK, quoting from commit 89d35528d17d > ("netlink: Add new socket option to enable strict checking on dumps"), > is used to "request strict checking of headers and attributes on dump > requests". > > If some attributes are set (including flags), setting this option causes > dump functions to filter results according to these attributes, via the > filter_set flag. However, if strict checking is requested, this should > imply that we also filter results based on flags that are *not* set. I don't agree with that comment. If a request does not specify a bit or specify an attribute on the request, it is a wildcard in the sense of nothing to be considered when matching records to be returned. > > This is currently not the case, at least for IPv4 FIB dumps: if the > RTM_F_CLONED flag is not set, and strict checking is required, we should > not return routes with the RTM_F_CLONED flag set. IPv4 currently ignores the CLONED flag and just returns - regardless of whether strict checking is enabled. This is the original short cut added many years ago. > > Set the filter_set flag whenever strict checking is requested, limiting > the scope to IPv4 FIB dumps for the moment being, as other users of the > flag might not present this inconsistency. > > Note that this partially duplicates the semantics of NLM_F_MATCH as > described by RFC 3549, par. 3.1.1. Instead of setting a filter based on > the size of the netlink message, properly support NLM_F_MATCH, by > setting a filter via ip_filter_fib_dump_req() and setting the filter_set > flag. > your commit description is very confusing given the end goal. can you explain again?
On Fri, 14 Jun 2019 21:13:38 -0600 David Ahern <dsahern@gmail.com> wrote: > On 6/14/19 7:32 PM, Stefano Brivio wrote: > > Socket option NETLINK_GET_STRICT_CHK, quoting from commit 89d35528d17d > > ("netlink: Add new socket option to enable strict checking on dumps"), > > is used to "request strict checking of headers and attributes on dump > > requests". > > > > If some attributes are set (including flags), setting this option causes > > dump functions to filter results according to these attributes, via the > > filter_set flag. However, if strict checking is requested, this should > > imply that we also filter results based on flags that are *not* set. > > I don't agree with that comment. If a request does not specify a bit or > specify an attribute on the request, it is a wildcard in the sense of > nothing to be considered when matching records to be returned. This is what I had in v1. Then: On Thu, 6 Jun 2019 16:47:00 -0600 David Ahern <dsahern@gmail.com> wrote: > That's the use case I was targeting: > 1. fib dumps - RTM_F_CLONED not set > 2. exception dump - RTM_F_CLONED set On Mon, 10 Jun 2019 15:38:06 -0600 David Ahern <dsahern@gmail.com> wrote: > By that I mean without the CLONED flag, no exceptions are returned > (default FIB dump). With the CLONED flag only exceptions are returned. and this looks to me like a sensible way (if strict checking is requested, or if NLM_F_MATCH is passed) to filter the results. > > This is currently not the case, at least for IPv4 FIB dumps: if the > > RTM_F_CLONED flag is not set, and strict checking is required, we should > > not return routes with the RTM_F_CLONED flag set. > > IPv4 currently ignores the CLONED flag and just returns - regardless of > whether strict checking is enabled. This is the original short cut added > many years ago. Sure, and I'm removing that, because there's no way to fetch cached routes otherwise. > > Set the filter_set flag whenever strict checking is requested, limiting > > the scope to IPv4 FIB dumps for the moment being, as other users of the > > flag might not present this inconsistency. > > > > Note that this partially duplicates the semantics of NLM_F_MATCH as > > described by RFC 3549, par. 3.1.1. Instead of setting a filter based on > > the size of the netlink message, properly support NLM_F_MATCH, by > > setting a filter via ip_filter_fib_dump_req() and setting the filter_set > > flag. > > > > your commit description is very confusing given the end goal. can you > explain again? 1. we need a way to filter on cached routes 2. RTM_F_CLONED, by itself, doesn't specify a filter 3. how do we turn that into a filter? NLM_F_MATCH, says RFC 3549 4. but if strict checking is requested, you also turn some attributes and flags into filters -- so let's make that apply to RTM_F_CLONED too, I don't see any reason why that should be special
On 6/14/19 9:23 PM, Stefano Brivio wrote: > > 1. we need a way to filter on cached routes > > 2. RTM_F_CLONED, by itself, doesn't specify a filter > > 3. how do we turn that into a filter? NLM_F_MATCH, says RFC 3549 > > 4. but if strict checking is requested, you also turn some attributes > and flags into filters -- so let's make that apply to RTM_F_CLONED > too, I don't see any reason why that should be special > I guess I am arguing (and Martin seems to agree with end goal) that RTM_F_CLONED is special. There are really 2 "databases" to be dumped here: FIB entries and exceptions. Which one to dump is controlled by RTM_F_CLONED.
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index 873fc5c4721c..32a04318d725 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -954,10 +954,14 @@ static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb) err = ip_filter_fib_dump_req(net, nlh, &filter, cb, true); if (err < 0) return err; - } else if (nlmsg_len(nlh) >= sizeof(struct rtmsg)) { - struct rtmsg *rtm = nlmsg_data(nlh); - - filter.flags = rtm->rtm_flags & (RTM_F_PREFIX | RTM_F_CLONED); + filter.filter_set = 1; + } else if (nlh->nlmsg_flags & NLM_F_MATCH) { + err = ip_filter_fib_dump_req(net, nlh, &filter, cb, false); + if (err == -ENODEV) + return skb->len; + if (err) + return err; + filter.filter_set = 1; } /* fib entries are never clones and ipv4 does not use prefix flag */
Socket option NETLINK_GET_STRICT_CHK, quoting from commit 89d35528d17d ("netlink: Add new socket option to enable strict checking on dumps"), is used to "request strict checking of headers and attributes on dump requests". If some attributes are set (including flags), setting this option causes dump functions to filter results according to these attributes, via the filter_set flag. However, if strict checking is requested, this should imply that we also filter results based on flags that are *not* set. This is currently not the case, at least for IPv4 FIB dumps: if the RTM_F_CLONED flag is not set, and strict checking is required, we should not return routes with the RTM_F_CLONED flag set. Set the filter_set flag whenever strict checking is requested, limiting the scope to IPv4 FIB dumps for the moment being, as other users of the flag might not present this inconsistency. Note that this partially duplicates the semantics of NLM_F_MATCH as described by RFC 3549, par. 3.1.1. Instead of setting a filter based on the size of the netlink message, properly support NLM_F_MATCH, by setting a filter via ip_filter_fib_dump_req() and setting the filter_set flag. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- v4: New patch net/ipv4/fib_frontend.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)