Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754211AbeAGQ31 (ORCPT + 1 other); Sun, 7 Jan 2018 11:29:27 -0500 Received: from mail-wr0-f196.google.com ([209.85.128.196]:40909 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754067AbeAGQ3O (ORCPT ); Sun, 7 Jan 2018 11:29:14 -0500 X-Google-Smtp-Source: ACJfBosTRCuIz4tVdg5zzREfEOmJHsYrJqcc/99j0KYsh722gAcOY4gEt0Uog47pVUu2VgL6m5SHWQ== Date: Sun, 7 Jan 2018 17:29:04 +0100 From: Ahmed Abdelsalam To: Pablo Neira Ayuso Cc: kadlec@blackhole.kfki.hu, fw@strlen.de, davem@davemloft.net, kuznet@ms2.inr.ac.ru, yoshfuji@linux-ipv6.org, linux-kernel@vger.kernel.org, netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org Subject: Re: [net-next] netfilter: add segment routing header 'srh' match Message-Id: <20180107172904.d01221bbec9bb04f25f5a6d0@gmail.com> In-Reply-To: <20180106234003.p2gpmvzsqvws57p5@salvia> References: <1514545672-13827-1-git-send-email-amsalam20@gmail.com> <20180106234003.p2gpmvzsqvws57p5@salvia> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.21; x86_64-apple-darwin10.8.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Sun, 7 Jan 2018 00:40:03 +0100 Pablo Neira Ayuso wrote: > Hi Ahmed, > > On Fri, Dec 29, 2017 at 12:07:52PM +0100, Ahmed Abdelsalam wrote: > > It allows matching packets based on Segment Routing Header > > (SRH) information. > > The implementation considers revision 7 of the SRH draft. > > https://tools.ietf.org/html/draft-ietf-6man-segment-routing-header-07 > > > > Currently supported match options include: > > (1) Next Header > > (2) Hdr Ext Len > > (3) Segments Left > > (4) Last Entry > > (5) Tag value of SRH > > > > Signed-off-by: Ahmed Abdelsalam > > --- > > include/uapi/linux/netfilter_ipv6/ip6t_srh.h | 63 ++++++++++ > > net/ipv6/netfilter/Kconfig | 9 ++ > > net/ipv6/netfilter/Makefile | 1 + > > net/ipv6/netfilter/ip6t_srh.c | 165 +++++++++++++++++++++++++++ > > 4 files changed, 238 insertions(+) > > create mode 100644 include/uapi/linux/netfilter_ipv6/ip6t_srh.h > > create mode 100644 net/ipv6/netfilter/ip6t_srh.c > > > > diff --git a/include/uapi/linux/netfilter_ipv6/ip6t_srh.h b/include/uapi/linux/netfilter_ipv6/ip6t_srh.h > > new file mode 100644 > > index 0000000..1b5dbd8 > > --- /dev/null > > +++ b/include/uapi/linux/netfilter_ipv6/ip6t_srh.h > > @@ -0,0 +1,63 @@ > > +/** > > + * Definitions for Segment Routing Header 'srh' match > > + * > > + * Author: > > + * Ahmed Abdelsalam > > + */ > > Please, add this in SPDX format instead. > > See include/uapi/linux/netfilter/xt_owner.h for instance. > Ok > > +#ifndef _IP6T_SRH_H > > +#define _IP6T_SRH_H > > + > > +#include > > +#include > > + > > +/* Values for "mt_flags" field in struct ip6t_srh */ > > +#define IP6T_SRH_NEXTHDR 0x0001 > > +#define IP6T_SRH_LEN_EQ 0x0002 > > +#define IP6T_SRH_LEN_GT 0x0004 > > +#define IP6T_SRH_LEN_LT 0x0008 > > +#define IP6T_SRH_SEGS_EQ 0x0010 > > +#define IP6T_SRH_SEGS_GT 0x0020 > > +#define IP6T_SRH_SEGS_LT 0x0040 > > +#define IP6T_SRH_LAST_EQ 0x0080 > > +#define IP6T_SRH_LAST_GT 0x0100 > > +#define IP6T_SRH_LAST_LT 0x0200 > > +#define IP6T_SRH_TAG 0x0400 > > +#define IP6T_SRH_MASK 0x07FF > > + > > +/* Values for "mt_invflags" field in struct ip6t_srh */ > > +#define IP6T_SRH_INV_NEXTHDR 0x0001 > > +#define IP6T_SRH_INV_LEN_EQ 0x0002 > > +#define IP6T_SRH_INV_LEN_GT 0x0004 > > +#define IP6T_SRH_INV_LEN_LT 0x0008 > > +#define IP6T_SRH_INV_SEGS_EQ 0x0010 > > +#define IP6T_SRH_INV_SEGS_GT 0x0020 > > +#define IP6T_SRH_INV_SEGS_LT 0x0040 > > +#define IP6T_SRH_INV_LAST_EQ 0x0080 > > +#define IP6T_SRH_INV_LAST_GT 0x0100 > > +#define IP6T_SRH_INV_LAST_LT 0x0200 > > +#define IP6T_SRH_INV_TAG 0x0400 > > +#define IP6T_SRH_INV_MASK 0x07FF > > Looking at all these EQ, GT, LT... I think this should be very easy to > implement in nf_tables with no kernel changes. > > You only need to add the protocol definition to: > > nftables/src/exthdr.c > > Would you have a look into this? This would be very much appreciated > to we keep nftables in sync with what we have in iptables. Yes, I look into it. I will send you a patch for nf_tables as well. > > > + > > +/** > > + * struct ip6t_srh - SRH match options > > + * @ next_hdr: Next header field of SRH > > + * @ hdr_len: Extension header length field of SRH > > + * @ segs_left: Segments left field of SRH > > + * @ last_entry: Last entry field of SRH > > + * @ tag: Tag field of SRH > > + * @ mt_flags: match options > > + * @ mt_invflags: Invert the sense of match options > > + */ > > + > > +struct ip6t_srh { > > + __u8 next_hdr; > > + __u8 hdr_len; > > + __u8 segs_left; > > + __u8 last_entry; > > + __u16 tag; > > + __u16 mt_flags; > > + __u16 mt_invflags; > > +}; > > + > > +#endif /*_IP6T_SRH_H*/ > > diff --git a/net/ipv6/netfilter/Kconfig b/net/ipv6/netfilter/Kconfig > > index 6acb2ee..e1818eb 100644 > > --- a/net/ipv6/netfilter/Kconfig > > +++ b/net/ipv6/netfilter/Kconfig > > @@ -232,6 +232,15 @@ config IP6_NF_MATCH_RT > > > > To compile it as a module, choose M here. If unsure, say N. > > > > +config IP6_NF_MATCH_SRH > > + tristate '"srh" Segment Routing header match support' > > + depends on NETFILTER_ADVANCED > > + help > > + srh matching allows you to match packets based on the segment > > + routing header of the packet. > > + > > + To compile it as a module, choose M here. If unsure, say N. > > + > > # The targets > > config IP6_NF_TARGET_HL > > tristate '"HL" hoplimit target support' > > diff --git a/net/ipv6/netfilter/Makefile b/net/ipv6/netfilter/Makefile > > index c6ee0cd..e0d51a9 100644 > > --- a/net/ipv6/netfilter/Makefile > > +++ b/net/ipv6/netfilter/Makefile > > @@ -54,6 +54,7 @@ obj-$(CONFIG_IP6_NF_MATCH_MH) += ip6t_mh.o > > obj-$(CONFIG_IP6_NF_MATCH_OPTS) += ip6t_hbh.o > > obj-$(CONFIG_IP6_NF_MATCH_RPFILTER) += ip6t_rpfilter.o > > obj-$(CONFIG_IP6_NF_MATCH_RT) += ip6t_rt.o > > +obj-$(CONFIG_IP6_NF_MATCH_SRH) += ip6t_srh.o > > > > # targets > > obj-$(CONFIG_IP6_NF_TARGET_MASQUERADE) += ip6t_MASQUERADE.o > > diff --git a/net/ipv6/netfilter/ip6t_srh.c b/net/ipv6/netfilter/ip6t_srh.c > > new file mode 100644 > > index 0000000..75e41dc9 > > --- /dev/null > > +++ b/net/ipv6/netfilter/ip6t_srh.c > > @@ -0,0 +1,165 @@ > > +/* > > + * Kernel module to match Segment Routing Header (SRH) parameters. > > + * > > + * Author: > > + * Ahmed Abdelsalam > > + * > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version 2 > > + * of the License, or (at your option) any later version. > > + */ > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("Xtables: IPv6 Segment Routing Header match"); > > +MODULE_AUTHOR("Ahmed Abdelsalam "); > > Not a deal breaker, but modern code usually places these MODULE_* > lines at the end of the file. > I will move them to the end. > > + > > +/* Test a struct->mt_invflags and a boolean for inequality */ > > +#define NF_SRH_INVF(ptr, flag, boolean) \ > > + ((boolean) ^ !!((ptr)->mt_invflags & (flag))) > > + > > +static bool srh_mt6(const struct sk_buff *skb, struct xt_action_param *par) > > +{ > > + const struct ip6t_srh *srhinfo = par->matchinfo; > > + struct ipv6_sr_hdr *srh; > > + struct ipv6_sr_hdr _srh; > > + int hdrlen, srhoff = 0; > > + > > + if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) > > + return false; > > + > > + srh = skb_header_pointer(skb, srhoff, sizeof(_srh), &_srh); > > + > > Remove unnecessary line break. > Ok > > + if (!srh) > > + return false; > > + > > + hdrlen = ipv6_optlen(srh); > > + if (skb->len - srhoff < hdrlen) > > + return false; > > + > > + if (srh->type != IPV6_SRCRT_TYPE_4) > > + return false; > > + > > + if (srh->segments_left > srh->first_segment) > > + return false; > > + > > + /* Next Header matching */ > > + if (srhinfo->mt_flags & IP6T_SRH_NEXTHDR) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_NEXTHDR, > > + !(srh->nexthdr == srhinfo->next_hdr))) > > + return false; > > + > > + /* Header Extension Length matching */ > > + if (srhinfo->mt_flags & IP6T_SRH_LEN_EQ) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LEN_EQ, > > + !(srh->hdrlen == srhinfo->hdr_len))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_LEN_GT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LEN_GT, > > + !(srh->hdrlen > srhinfo->hdr_len))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_LEN_LT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LEN_LT, > > + !(srh->hdrlen < srhinfo->hdr_len))) > > + return false; > > + > > + /* Segments Left matching */ > > + if (srhinfo->mt_flags & IP6T_SRH_SEGS_EQ) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_SEGS_EQ, > > + !(srh->segments_left == srhinfo->segs_left))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_SEGS_GT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_SEGS_GT, > > + !(srh->segments_left > srhinfo->segs_left))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_SEGS_LT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_SEGS_LT, > > + !(srh->segments_left < srhinfo->segs_left))) > > + return false; > > + > > + /** > > + * Last Entry matching > > + * Last_Entry field was introduced in revision 6 of the SRH draft. > > + * It was called First_Segment in the previous revision > > + */ > > + if (srhinfo->mt_flags & IP6T_SRH_LAST_EQ) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LAST_EQ, > > + !(srh->first_segment == srhinfo->last_entry))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_LAST_GT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LAST_GT, > > + !(srh->first_segment > srhinfo->last_entry))) > > + return false; > > + > > + if (srhinfo->mt_flags & IP6T_SRH_LAST_LT) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_LAST_LT, > > + !(srh->first_segment < srhinfo->last_entry))) > > + return false; > > + > > + /** > > + * Tag matchig > > + * Tag field was introduced in revision 6 of the SRH draft. > > + */ > > + if (srhinfo->mt_flags & IP6T_SRH_TAG) > > + if (NF_SRH_INVF(srhinfo, IP6T_SRH_INV_TAG, > > + !(srh->tag == srhinfo->tag))) > > + return false; > > + return true; > > +} > > + > > +static int srh_mt6_check(const struct xt_mtchk_param *par) > > +{ > > + const struct ip6t_srh *srhinfo = par->matchinfo; > > + > > + if (srhinfo->mt_flags & ~IP6T_SRH_MASK) { > > + pr_debug("unknown srh match flags %X\n", srhinfo->mt_flags); > > Better call pr_err() here. I know we don't do this in other > extensions, but we should not use pr_debug() for this. ip6tables > explicit refers to 'dmesg' when -EINVAL is returned. > I will change this one also . > Thanks. I will send a new pacth addressing these comments. Thanks -- Ahmed Abdelsalam