netdev - Re: [PATCH iproute2] Add "show" subcommand to "ip fou"

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <B4217A1A-AD82-459A-AAC8-C321EEBAA2DC@apple.com>
Date:   Thu, 16 Nov 2017 12:59:28 -0800
From:   Greg Greenway <ggreenway@...le.com>
To:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH iproute2] Add "show" subcommand to "ip fou"


> On Nov 3, 2017, at 10:25 AM, Tom Herbert <tom@...bertland.com> wrote:
> 
> On Fri, Nov 3, 2017 at 10:19 AM, Greg Greenway <ggreenway@...le.com> wrote:
>> On Nov 1, 2017, at 2:03 PM, Stephen Hemminger <stephen@...workplumber.org> wrote:
>>> 
>>> On Tue, 31 Oct 2017 13:00:47 -0700
>>> Greg Greenway <ggreenway@...le.com> wrote:
>>> 
>>>> +    if (tb[FOU_ATTR_AF]) {
>>>> +            family = rta_getattr_u8(tb[FOU_ATTR_AF]);
>>>> +            if (family == AF_INET)
>>>> +                    family_str = "AF_INET";
>>>> +            else if (family == AF_INET6)
>>>> +                    family_str = "AF_INET6";
>>>> +            else
>>>> +                    family_str = "unknown";
>>>> +            fprintf(fp, "af %s ", family_str);
>>> 
>>> The unwritten rule for ip commands is that the show function
>>> must format the output with same command syntax as the other commands set/add/delete.
>>> Since there is no "af AF_INET" option to ip fou, this breaks that convention.
>>> Either ignore the address family, change the add command, or output with same
>>> syntax (-6); preferably the latter.
>> 
>> That makes sense.  Here's a corrected version.  It also avoids a trailing-space in the output.
>> 
>> From: Greg Greenway <ggreenway@...le.com>
>> Date: Tue, 31 Oct 2017 12:47:35 -0700
>> Subject: [PATCH] Add "show" subcommand to "ip fou".
>> 
>> Sample output:
>> 
>> $ sudo ./ip/ip fou add port 111 ipproto 11
>> $ sudo ./ip/ip fou add port 222 ipproto 22 -6
>> $ ./ip/ip fou show
>> port 222 ipproto 22 -6
>> port 111 ipproto 11
>> 
>> Signed-off-by: Greg Greenway <ggreenway@...le.com>
>> ---
>> ip/ipfou.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>> 
>> diff --git a/ip/ipfou.c b/ip/ipfou.c
>> index 00dbe15..ecbaf11 100644
>> --- a/ip/ipfou.c
>> +++ b/ip/ipfou.c
>> @@ -28,6 +28,7 @@ static void usage(void)
>>        fprintf(stderr, "Usage: ip fou add port PORT "
>>                "{ ipproto PROTO  | gue } [ -6 ]\n");
>>        fprintf(stderr, "       ip fou del port PORT [ -6 ]\n");
>> +       fprintf(stderr, "       ip fou show\n");
>>        fprintf(stderr, "\n");
>>        fprintf(stderr, "Where: PROTO { ipproto-name | 1..255 }\n");
>>        fprintf(stderr, "       PORT { 1..65535 }\n");
>> @@ -134,6 +135,63 @@ static int do_del(int argc, char **argv)
>>        return 0;
>> }
>> 
>> +static int print_fou_mapping(const struct sockaddr_nl *who,
>> +                                struct nlmsghdr *n, void *arg)
>> +{
>> +       FILE *fp = (FILE *)arg;
>> +       struct genlmsghdr *ghdr;
>> +       struct rtattr *tb[FOU_ATTR_MAX + 1];
>> +       int len = n->nlmsg_len;
>> +       unsigned family;
>> +
>> +       if (n->nlmsg_type != genl_family)
>> +               return 0;
>> +
>> +       len -= NLMSG_LENGTH(GENL_HDRLEN);
>> +       if (len < 0)
>> +               return -1;
>> +
>> +       ghdr = NLMSG_DATA(n);
>> +       parse_rtattr(tb, FOU_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len);
>> +
>> +       if (tb[FOU_ATTR_PORT])
>> +               fprintf(fp, "port %u", ntohs(rta_getattr_u16(tb[FOU_ATTR_PORT])));
>> +       if (tb[FOU_ATTR_TYPE] && rta_getattr_u8(tb[FOU_ATTR_TYPE]) == FOU_ENCAP_GUE)
>> +               fprintf(fp, " gue");
>> +       else if (tb[FOU_ATTR_IPPROTO])
>> +               fprintf(fp, " ipproto %u", rta_getattr_u8(tb[FOU_ATTR_IPPROTO]));
>> +       if (tb[FOU_ATTR_AF]) {
>> +               family = rta_getattr_u8(tb[FOU_ATTR_AF]);
>> +               if (family == AF_INET6)
>> +                       fprintf(fp, " -6");
>> +       }
>> +       fprintf(fp, "\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static int do_show(int argc, char **argv)
>> +{
>> +       FOU_REQUEST(req, 4096, FOU_CMD_GET, NLM_F_REQUEST | NLM_F_DUMP);
>> +
>> +       if (argc > 0) {
>> +               fprintf(stderr, "\"ip fou show\" does not take any arguments.\n");
>> +               return -1;
>> +       }
>> +
>> +       if (rtnl_send(&genl_rth, &req.n, req.n.nlmsg_len) < 0) {
>> +               perror("Cannot send show request");
>> +               exit(1);
>> +       }
>> +
>> +       if (rtnl_dump_filter(&genl_rth, print_fou_mapping, stdout) < 0) {
>> +               fprintf(stderr, "Dump terminated\n");
>> +               return 1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> int do_ipfou(int argc, char **argv)
>> {
>>        if (argc < 1)
>> @@ -149,6 +207,8 @@ int do_ipfou(int argc, char **argv)
>>                return do_add(argc-1, argv+1);
>>        if (matches(*argv, "delete") == 0)
>>                return do_del(argc-1, argv+1);
>> +       if (matches(*argv, "show") == 0)
>> +               return do_show(argc-1, argv+1);
>>        fprintf(stderr, "Command \"%s\" is unknown, try \"ip fou help\".\n", *argv);
>>        exit(-1);
>> }
>> --
>> 2.7.4
>> 
> Acked-by: Tom Herbert <tom@...bertland.com>

Are there any other issues/concerns, or anything else I need to do for this patch to be accepted?

Thanks,
Greg