lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 14 Mar 2018 21:26:40 +0100 From: Alexander Zubkov <green@....ru> To: Serhey Popovych <serhe.popovych@...il.com>, Luca Boccassi <bluca@...ian.org>, Stephen Hemminger <stephen@...workplumber.org> Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org> Subject: Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Hello, For example, it can be fixed in such way (patch is below): - split handling of default and all/any - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default - and AF_UNSPEC for all/any In this case "ip route show default" shows only default route and "ip route show all" shows all routes. And both also work when family (-4 or -6) is specified. Serhey, does it goes in line with what you wanted to achieve? Because I do not know - may be there are reasons why all/any should be provided with specific family. If you think this solution is suitable, I'll do some additional tests and package the patch in a proper way for this mailing list. And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both branches. May be someone have some additional thoughts on that? --- a/lib/utils.c +++ b/lib/utils.c @@ -560,14 +560,23 @@ static int __get_addr_1(inet_prefix *addr, const char *name, int family) { memset(addr, 0, sizeof(*addr)); - if (strcmp(name, "default") == 0 || - strcmp(name, "all") == 0 || - strcmp(name, "any") == 0) { + if (strcmp(name, "default") == 0) { if ((family == AF_DECnet) || (family == AF_MPLS)) return -1; addr->family = (family != AF_UNSPEC) ? family : AF_INET; addr->bytelen = af_byte_len(addr->family); addr->bitlen = -2; + addr->flags |= PREFIXLEN_SPECIFIED; + return 0; + } + + if (strcmp(name, "all") == 0 || + strcmp(name, "any") == 0) { + if ((family == AF_DECnet) || (family == AF_MPLS)) + return -1; + addr->family = AF_UNSPEC; + addr->bytelen = 0; + addr->bitlen = -2; return 0; } @@ -695,7 +704,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) bitlen = af_bit_len(dst->family); - flags = PREFIXLEN_SPECIFIED; + flags = 0; if (slash) { unsigned int plen; @@ -706,12 +715,11 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) if (plen > bitlen) return -1; + flags |= PREFIXLEN_SPECIFIED; bitlen = plen; } else { if (dst->bitlen == -2) bitlen = 0; - else - flags = 0; } dst->flags |= flags; On 14.03.2018 09:59, Alexander Zubkov wrote: > Hello, > > There was a series of patches by Serhey and specifically this one: > https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=93fa12418dc6f5943692250244be303bb162175b > > It drops handling of special prefix names in get_prefix_1(), and in get_addr_1() they always receive family and bytelen. But as I unerstand for any case it was important to keep it with unspecified family for further filtering. As I do not know what is the global idea, I want to discuss it. Because there are options depending on how and where we want to handle those special names. Like keep unspecified family or change filtering logic. > > I have added Serhey Popovych in the recepients, so he can give some ideas on what his aim is and help choose better solution. > > 13.03.2018, 21:12, "Alexander Zubkov" <green@....ru>: >> Hi, >> >> I just realized that you need patch for v4.15.0, which is easier to do. >> I'll send it as separate message now. I will make patch for the master >> branch, but later. >> >> On 13.03.2018 13:02, Luca Boccassi wrote: >>> On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: >>>> Hello again, >>>> >>>> The fun thing is that before the commit "ip route ls all" showed all >>>> routes, but "ip -[4|6] route ls all" showed only default. So it was >>>> broken too, but in other way. >>>> I see parsing of prefix was changed since my patch. So I need several >>>> days to propose fix. I think if "ip route ls [all|any]" shows all >>>> routes and "ip route ls default" shows only default, everybody will >>>> be happy with that? >>> >>> Hi, >>> >>> My only concern is that behaviour of existing commands that have been >>> in releases is not changed, otherwise I get bugs raised :-) >>> >>> Thank you for your work! >>> >>>> 13.03.2018, 09:46, "Alexander Zubkov" <green@....ru>: >>>>> Hello. >>>>> >>>>> May be the better way would be to change how "all"/"any" argument >>>>> behaves? My original concern was about "default" only. I agree too, >>>>> that "all" or "any" should work for all routes. But not for the >>>>> default. >>>>> >>>>> 12.03.2018, 22:37, "Luca Boccassi" <bluca@...ian.org>: >>>>>> On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >>>>>>> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >>>>>>> >>>>>>> Debian maintainer found that basic command: >>>>>>> # ip route flush all >>>>>>> No longer worked as expected which breaks user scripts and >>>>>>> expectations. It no longer flushed all IPv4 routes. >>>>>>> >>>>>>> Reported-by: Luca Boccassi <bluca@...ian.org> >>>>>>> Signed-off-by: Stephen Hemminger <stephen@...workplumber.org> >>>>>>> --- >>>>>>> ip/iproute.c | 65 ++++++++++++++++++---------------------- >>>>>>> -------- >>>>>>> ------------ >>>>>>> lib/utils.c | 13 ++++++++++++ >>>>>>> 2 files changed, 32 insertions(+), 46 deletions(-) >>>>>> >>>>>> Tested-by: Luca Boccassi <bluca@...ian.org> >>>>>> >>>>>> Thanks, solves the problem. I'll backport it to Debian. >>>>>> >>>>>> Alexander, reproducing the issue is quite simple - before that >>>>>> commit, >>>>>> ip route ls all showed all routes, but with the change it >>>>>> started >>>>>> showing only the default table. Same for ip route flush. >>>>>> >>>>>> -- >>>>>> Kind regards, >>>>>> Luca Boccassi
Powered by blists - more mailing lists