Re: [PATCH] ebtables: Fix build errors and warnings

2018-05-20 Thread Duncan Roe 
On Tue, May 15, 2018 at 12:42:09AM +0200, Florian Westphal wrote:
> Duncan Roe  wrote:
> > Since commit b1cdae87f25021eb835872d86d6e7206bd421c3f, make fails thusly:
> >
> > > libebtc.c: In function 'ebt_reinit_extensions':
> > > libebtc.c:275:11: error: 'union ' has no member named 
> > > 'revision'
> > > m->m->u.revision = m->revision;
> > >^
> > > libebtc.c: In function 'ebt_check_rule_exists':
> > > libebtc.c:555:21: error: 'union ' has no member named 
> > > 'revision'
> > >m_l2->m->u.revision != m->m->u.revision)) {
> > >  ^
> > > libebtc.c:555:41: error: 'union ' has no member named 
> > > 'revision'
> > >m_l2->m->u.revision != m->m->u.revision)) {
> > >  ^
> > > libebtc.c: In function 'ebt_register_match':
> > > libebtc.c:1215:9: error: 'union ' has no member named 
> > > 'revision'
> > >   m->m->u.revision = m->revision;
> > >  ^
> > The cause of this failure is that the commit updated include/ebtables.h but
> > libebtc.c uses include/linux/netfilter_bridge/ebtables.h via
> > include/ebtables_u.h (gcc -E -C verifies this).
> >
> > The 2 versions of ebtables.h looked to me to be otherwise close enough, so
> > amended ebtables_u.h to use the newer one.
> >
> > Makefile insists on being warning-free, so cleared up warnings. Apart from
> > unused variables, there was also the issue that the diagnostic macro
> > ebt_print_error2 *returns* (i.e. makes its caller return) and returns -1. 
> > This
> > is unsuitable for use in functions which do not return a value, so 
> > introduced
> > ebt_print_error3 to do this.
>
> Applied, thanks Duncan.
> I made one change, build still failed for me on FC28 due to strncpy
> warning:
>
> extensions/ebt_string.c:171:3: error: ???strncpy??? specified bound 16
> equals destination size [-Werror=stringop-truncation]
> strncpy(info->algo, optarg, XT_STRING_MAX_ALGO_NAME_SIZE);
>^

Odd that. I didn't see this line being changed, but am not getting a warning
from gcc8.1 (that FC28 has, right?).

Cheers ... Duncan.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/14] net: sched: use unique idr insert function in unlocked actions

2018-05-20 Thread Marcelo Ricardo Leitner 
On Mon, May 21, 2018 at 12:40:54AM +0300, Or Gerlitz wrote:
> On Mon, May 21, 2018 at 12:33 AM, Marcelo Ricardo Leitner
>  wrote:
> > On Mon, May 21, 2018 at 12:13:06AM +0300, Or Gerlitz wrote:
> >> On Sun, May 20, 2018 at 1:20 AM, Marcelo Ricardo Leitner
> >>  wrote:
> >> > On Mon, May 14, 2018 at 05:27:14PM +0300, Vlad Buslov wrote:
> >> >> Substitute calls to action insert function with calls to action insert
> >> >> unique function that warns if insertion overwrites index in idr.
> >> >
> >> > I know this patch may be gone on V2, but a general comment, please use
> >> > the function names themselves instead of a textualized version. I.e.,
> >> > s/action insert unique/tcf_idr_insert_unique/
> >>
> >> disagree. While doing reviews I found out that if I ask the developer
> >> to use higher
> >> level / descriptive language and specifically to avoid putting
> >> variable / function names in
> >> patch titles and change logs, the quality gets ++ big time, vs if the
> >> developer is allowed to say
> >>
> >> net/mlx5: Changed add_vovo_bobo()
> >>
> >> Added variable do_it_right to add_vovo_bobo(), now we are terribly good.
> >
> > In your example I agree that it is not helping and it is even allowing
> > such empty changelog, just as in the section I highlighted, the
> > descriptive language is also not helping IMHO.
> >
> > I had to read it 3 times to make sure I wasn't missing a modifier word
> > when comparing the two functions and well, it's just saying
> > "Substitute calls to foo function to bar function". I don't see how
> > the textualized version helps in this case while, at least in this
> > one, I would have visually recognized the function names way faster.
> >
> > Sounds like 2 bad examples for either approach.
>
> Properly written descriptive language is maybe harder to come up with
> (specifically for those of us who are not native english speakers, like me)
> but is more expressive and helpful for reviews and maintenance. Lets try
> to add Vlad to come up with the right higher language and not ask him to
> quote function and variable named.

Alright.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH xtables] ebtables-compat: add arp extension

2018-05-20 Thread Florian Westphal 
no translation yet, might be doable with raw payload expressions though.

Signed-off-by: Florian Westphal 
---
 extensions/libebt_arp.c | 490 
 iptables/xtables-eb.c   |   1 +
 2 files changed, 491 insertions(+)
 create mode 100644 extensions/libebt_arp.c

diff --git a/extensions/libebt_arp.c b/extensions/libebt_arp.c
new file mode 100644
index ..45fc8d73e24d
--- /dev/null
+++ b/extensions/libebt_arp.c
@@ -0,0 +1,490 @@
+/* ebt_arp
+ *
+ * Authors:
+ * Bart De Schuymer 
+ * Tim Gardner 
+ *
+ * April, 2002
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include "iptables/nft.h"
+#include "iptables/nft-bridge.h"
+
+#define ARP_OPCODE '1'
+#define ARP_HTYPE  '2'
+#define ARP_PTYPE  '3'
+#define ARP_IP_S   '4'
+#define ARP_IP_D   '5'
+#define ARP_MAC_S  '6'
+#define ARP_MAC_D  '7'
+#define ARP_GRAT   '8'
+
+static const struct option brarp_opts[] = {
+   { "arp-opcode", required_argument, 0, ARP_OPCODE },
+   { "arp-op", required_argument, 0, ARP_OPCODE },
+   { "arp-htype" , required_argument, 0, ARP_HTYPE  },
+   { "arp-ptype" , required_argument, 0, ARP_PTYPE  },
+   { "arp-ip-src", required_argument, 0, ARP_IP_S   },
+   { "arp-ip-dst", required_argument, 0, ARP_IP_D   },
+   { "arp-mac-src"   , required_argument, 0, ARP_MAC_S  },
+   { "arp-mac-dst"   , required_argument, 0, ARP_MAC_D  },
+   { "arp-gratuitous",   no_argument, 0, ARP_GRAT   },
+   XT_GETOPT_TABLEEND,
+};
+
+/* a few names */
+static char *opcodes[] =
+{
+   "Request",
+   "Reply",
+   "Request_Reverse",
+   "Reply_Reverse",
+   "DRARP_Request",
+   "DRARP_Reply",
+   "DRARP_Error",
+   "InARP_Request",
+   "ARP_NAK",
+};
+
+static void brarp_print_help(void)
+{
+   int i;
+
+   printf(
+"arp options:\n"
+"--arp-opcode  [!] opcode: ARP opcode (integer or string)\n"
+"--arp-htype   [!] type  : ARP hardware type (integer or string)\n"
+"--arp-ptype   [!] type  : ARP protocol type (hexadecimal or string)\n"
+"--arp-ip-src  [!] address[/mask]: ARP IP source specification\n"
+"--arp-ip-dst  [!] address[/mask]: ARP IP target specification\n"
+"--arp-mac-src [!] address[/mask]: ARP MAC source specification\n"
+"--arp-mac-dst [!] address[/mask]: ARP MAC target specification\n"
+"[!] --arp-gratuitous: ARP gratuitous packet\n"
+" opcode strings: \n");
+   for (i = 0; i < ARRAY_SIZE(opcodes); i++)
+   printf(" %d = %s\n", i + 1, opcodes[i]);
+   printf(
+" hardware type string: 1 = Ethernet\n"
+" protocol type string: see "_PATH_ETHERTYPES"\n");
+}
+
+#define OPT_OPCODE 0x01
+#define OPT_HTYPE  0x02
+#define OPT_PTYPE  0x04
+#define OPT_IP_S   0x08
+#define OPT_IP_D   0x10
+#define OPT_MAC_S  0x20
+#define OPT_MAC_D  0x40
+#define OPT_GRAT   0x80
+
+static int undot_ip(char *ip, unsigned char *ip2)
+{
+   char *p, *q, *end;
+   long int onebyte;
+   int i;
+   char buf[20];
+
+   strncpy(buf, ip, sizeof(buf) - 1);
+
+   p = buf;
+   for (i = 0; i < 3; i++) {
+   if ((q = strchr(p, '.')) == NULL)
+   return -1;
+   *q = '\0';
+   onebyte = strtol(p, , 10);
+   if (*end != '\0' || onebyte > 255 || onebyte < 0)
+   return -1;
+   ip2[i] = (unsigned char)onebyte;
+   p = q + 1;
+   }
+
+   onebyte = strtol(p, , 10);
+   if (*end != '\0' || onebyte > 255 || onebyte < 0)
+   return -1;
+   ip2[3] = (unsigned char)onebyte;
+
+   return 0;
+}
+
+static int ip_mask(char *mask, unsigned char *mask2)
+{
+   char *end;
+   long int bits;
+   uint32_t mask22;
+
+   if (undot_ip(mask, mask2)) {
+   /* not the /a.b.c.e format, maybe the /x format */
+   bits = strtol(mask, , 10);
+   if (*end != '\0' || bits > 32 || bits < 0)
+   return -1;
+   if (bits != 0) {
+   mask22 = htonl(0x << (32 - bits));
+   memcpy(mask2, , 4);
+   } else {
+   mask22 = 0x;
+   memcpy(mask2, , 4);
+   }
+   }
+   return 0;
+}
+
+static void ebt_parse_ip_address(char *address, uint32_t *addr, uint32_t *msk)
+{
+   char *p;
+
+   /* first the mask */
+   if ((p = strrchr(address, '/')) != NULL) {
+   *p = '\0';
+   if (ip_mask(p + 1, (unsigned char *)msk)) {
+   xtables_error(PARAMETER_PROBLEM,
+ "Problem with the IP mask '%s'", p + 1);
+   return;
+   }
+   } else
+   *msk = 0x;
+
+   if (undot_ip(address, (unsigned char *)addr))

[PATCH xtables] ebtables-compat: add redirect match extension

2018-05-20 Thread Florian Westphal 
No translation.  The kernel match will alter packet type
(meta set pkttype), but also replace dst mac with the bridges' mac address,
however nft currently doesn't allow to retrieve this at runtime.

So just add this without the xlate part for now.

Signed-off-by: Florian Westphal 
---
 extensions/libebt_redirect.c | 109 +++
 iptables/xtables-eb.c|   1 +
 2 files changed, 110 insertions(+)
 create mode 100644 extensions/libebt_redirect.c

diff --git a/extensions/libebt_redirect.c b/extensions/libebt_redirect.c
new file mode 100644
index ..a88713d33145
--- /dev/null
+++ b/extensions/libebt_redirect.c
@@ -0,0 +1,109 @@
+/* ebt_redirect
+ *
+ * Authors:
+ * Bart De Schuymer 
+ *
+ * April, 2002
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "iptables/nft.h"
+#include "iptables/nft-bridge.h"
+
+#define REDIRECT_TARGET '1'
+static const struct option brredir_opts[] =
+{
+   { "redirect-target", required_argument, 0, REDIRECT_TARGET },
+   { 0 }
+};
+
+static void brredir_print_help(void)
+{
+   printf(
+   "redirect option:\n"
+   " --redirect-target target   : ACCEPT, DROP, RETURN or CONTINUE\n");
+}
+
+static void brredir_init(struct xt_entry_target *target)
+{
+   struct ebt_redirect_info *redirectinfo =
+  (struct ebt_redirect_info *)target->data;
+
+   redirectinfo->target = EBT_ACCEPT;
+}
+
+#define OPT_REDIRECT_TARGET  0x01
+static int brredir_parse(int c, char **argv, int invert, unsigned int *flags,
+const void *entry, struct xt_entry_target **target)
+{
+   struct ebt_redirect_info *redirectinfo =
+  (struct ebt_redirect_info *)(*target)->data;
+
+   switch (c) {
+   case REDIRECT_TARGET:
+   EBT_CHECK_OPTION(flags, OPT_REDIRECT_TARGET);
+   if (ebt_fill_target(optarg, (unsigned int 
*)>target))
+   xtables_error(PARAMETER_PROBLEM, "Illegal 
--redirect-target target");
+   break;
+   default:
+   return 0;
+   }
+   return 1;
+}
+
+static void brredir_print(const void *ip, const struct xt_entry_target 
*target, int numeric)
+{
+   struct ebt_redirect_info *redirectinfo =
+  (struct ebt_redirect_info *)target->data;
+
+   if (redirectinfo->target == EBT_ACCEPT)
+   return;
+   printf(" --redirect-target %s", ebt_target_name(redirectinfo->target));
+}
+
+static const char* brredir_verdict(int verdict)
+{
+   switch (verdict) {
+   case EBT_ACCEPT: return "accept";
+   case EBT_DROP: return "drop";
+   case EBT_CONTINUE: return "continue";
+   case EBT_RETURN: return "return";
+   }
+
+   return "";
+}
+
+static int brredir_xlate(struct xt_xlate *xl,
+const struct xt_xlate_tg_params *params)
+{
+   const struct ebt_redirect_info *red = (const void*)params->target->data;
+
+   xt_xlate_add(xl, "meta set pkttype host");
+   if (red->target != EBT_ACCEPT)
+   xt_xlate_add(xl, " %s ", brredir_verdict(red->target));
+   return 0;
+}
+
+static struct xtables_target brredirect_target = {
+   .name   = "redirect",
+   .version= XTABLES_VERSION,
+   .family = NFPROTO_BRIDGE,
+   .size   = XT_ALIGN(sizeof(struct ebt_redirect_info)),
+   .userspacesize  = XT_ALIGN(sizeof(struct ebt_redirect_info)),
+   .help   = brredir_print_help,
+   .init   = brredir_init,
+   .parse  = brredir_parse,
+   .print  = brredir_print,
+   .xlate  = brredir_xlate,
+   .extra_opts = brredir_opts,
+};
+
+void _init(void)
+{
+   xtables_register_target(_target);
+}
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index a72c90735eb0..1230125f98f2 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -678,6 +678,7 @@ void ebt_load_match_extensions(void)
ebt_load_target("mark");
ebt_load_target("dnat");
ebt_load_target("snat");
+   ebt_load_target("redirect");
 }
 
 void ebt_add_match(struct xtables_match *m,
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH xtables] ebtables-compat: add nat match extensions

2018-05-20 Thread Florian Westphal 
adds snat and dnat.

Translation for snat isn't complete, the --snat-arp switch isn't supported so 
far.

Signed-off-by: Florian Westphal 
---
 extensions/libebt_dnat.c  | 134 ++
 extensions/libebt_dnat.txlate |   8 ++
 extensions/libebt_snat.c  | 151 ++
 extensions/libebt_snat.txlate |   5 ++
 iptables/xtables-eb.c |   2 +
 5 files changed, 300 insertions(+)
 create mode 100644 extensions/libebt_dnat.c
 create mode 100644 extensions/libebt_dnat.txlate
 create mode 100644 extensions/libebt_snat.c
 create mode 100644 extensions/libebt_snat.txlate

diff --git a/extensions/libebt_dnat.c b/extensions/libebt_dnat.c
new file mode 100644
index ..c179d8c19bc1
--- /dev/null
+++ b/extensions/libebt_dnat.c
@@ -0,0 +1,134 @@
+/* ebt_nat
+ *
+ * Authors:
+ * Bart De Schuymer 
+ *
+ * June, 2002
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "iptables/nft.h"
+#include "iptables/nft-bridge.h"
+
+#define NAT_D '1'
+#define NAT_D_TARGET '2'
+static const struct option brdnat_opts[] =
+{
+   { "to-destination", required_argument, 0, NAT_D },
+   { "to-dst", required_argument, 0, NAT_D },
+   { "dnat-target"   , required_argument, 0, NAT_D_TARGET },
+   { 0 }
+};
+
+static void brdnat_print_help(void)
+{
+   printf(
+   "dnat options:\n"
+   " --to-dst address   : MAC address to map destination to\n"
+   " --dnat-target target   : ACCEPT, DROP, RETURN or CONTINUE\n");
+}
+
+static void brdnat_init(struct xt_entry_target *target)
+{
+   struct ebt_nat_info *natinfo = (struct ebt_nat_info *)target->data;
+
+   natinfo->target = EBT_ACCEPT;
+}
+
+#define OPT_DNAT0x01
+#define OPT_DNAT_TARGET 0x02
+static int brdnat_parse(int c, char **argv, int invert, unsigned int *flags,
+const void *entry, struct xt_entry_target **target)
+{
+   struct ebt_nat_info *natinfo = (struct ebt_nat_info *)(*target)->data;
+   struct ether_addr *addr;
+
+   switch (c) {
+   case NAT_D:
+   EBT_CHECK_OPTION(flags, OPT_DNAT);
+   if (!(addr = ether_aton(optarg)))
+   xtables_error(PARAMETER_PROBLEM, "Problem with 
specified --to-destination mac");
+   memcpy(natinfo->mac, addr, ETH_ALEN);
+   break;
+   case NAT_D_TARGET:
+   EBT_CHECK_OPTION(flags, OPT_DNAT_TARGET);
+   if (ebt_fill_target(optarg, (unsigned int *)>target))
+   xtables_error(PARAMETER_PROBLEM, "Illegal --dnat-target 
target");
+   break;
+   default:
+   return 0;
+   }
+   return 1;
+}
+
+static void brdnat_final_check(unsigned int flags)
+{
+   if (!flags)
+   xtables_error(PARAMETER_PROBLEM,
+ "You must specify proper arguments");
+}
+
+static void ebt_print_mac(const unsigned char *mac)
+{
+   printf("%s", ether_ntoa((struct ether_addr *) mac));
+}
+
+static void brdnat_print(const void *ip, const struct xt_entry_target *target, 
int numeric)
+{
+   struct ebt_nat_info *natinfo = (struct ebt_nat_info *)target->data;
+
+   printf("--to-dst ");
+   ebt_print_mac(natinfo->mac);
+   printf(" --dnat-target %s", ebt_target_name(natinfo->target));
+}
+
+static const char* brdnat_verdict(int verdict)
+{
+   switch (verdict) {
+   case EBT_ACCEPT: return "accept";
+   case EBT_DROP: return "drop";
+   case EBT_CONTINUE: return "continue";
+   case EBT_RETURN: return "return";
+   }
+
+   return "";
+}
+
+static int brdnat_xlate(struct xt_xlate *xl,
+const struct xt_xlate_tg_params *params)
+{
+   const struct ebt_nat_info *natinfo = (const void*)params->target->data;
+
+   xt_xlate_add(xl, "ether daddr set %s %s ",
+ether_ntoa((struct ether_addr *)natinfo->mac),
+brdnat_verdict(natinfo->target));
+
+   return 1;
+}
+
+static struct xtables_target brdnat_target =
+{
+   .name   = "dnat",
+   .version= XTABLES_VERSION,
+   .family = NFPROTO_BRIDGE,
+   .size   = XT_ALIGN(sizeof(struct ebt_nat_info)),
+   .userspacesize  = XT_ALIGN(sizeof(struct ebt_nat_info)),
+   .help   = brdnat_print_help,
+   .init   = brdnat_init,
+   .parse  = brdnat_parse,
+   .final_check= brdnat_final_check,
+   .print  = brdnat_print,
+   .xlate  = brdnat_xlate,
+   .extra_opts = brdnat_opts,
+};
+
+void _init(void)
+{
+   xtables_register_target(_target);
+}
diff --git a/extensions/libebt_dnat.txlate b/extensions/libebt_dnat.txlate
new file mode 100644
index ..2652dd55b264
--- /dev/null
+++ b/extensions/libebt_dnat.txlate
@@ -0,0 +1,8 @@
+ebtables-translate -t nat -A PREROUTING -i someport

Re: [PATCH 13/14] net: sched: use unique idr insert function in unlocked actions

2018-05-20 Thread Or Gerlitz 
On Mon, May 21, 2018 at 12:33 AM, Marcelo Ricardo Leitner
 wrote:
> On Mon, May 21, 2018 at 12:13:06AM +0300, Or Gerlitz wrote:
>> On Sun, May 20, 2018 at 1:20 AM, Marcelo Ricardo Leitner
>>  wrote:
>> > On Mon, May 14, 2018 at 05:27:14PM +0300, Vlad Buslov wrote:
>> >> Substitute calls to action insert function with calls to action insert
>> >> unique function that warns if insertion overwrites index in idr.
>> >
>> > I know this patch may be gone on V2, but a general comment, please use
>> > the function names themselves instead of a textualized version. I.e.,
>> > s/action insert unique/tcf_idr_insert_unique/
>>
>> disagree. While doing reviews I found out that if I ask the developer
>> to use higher
>> level / descriptive language and specifically to avoid putting
>> variable / function names in
>> patch titles and change logs, the quality gets ++ big time, vs if the
>> developer is allowed to say
>>
>> net/mlx5: Changed add_vovo_bobo()
>>
>> Added variable do_it_right to add_vovo_bobo(), now we are terribly good.
>
> In your example I agree that it is not helping and it is even allowing
> such empty changelog, just as in the section I highlighted, the
> descriptive language is also not helping IMHO.
>
> I had to read it 3 times to make sure I wasn't missing a modifier word
> when comparing the two functions and well, it's just saying
> "Substitute calls to foo function to bar function". I don't see how
> the textualized version helps in this case while, at least in this
> one, I would have visually recognized the function names way faster.
>
> Sounds like 2 bad examples for either approach.

Properly written descriptive language is maybe harder to come up with
(specifically for those of us who are not native english speakers, like me)
but is more expressive and helpful for reviews and maintenance. Lets try
to add Vlad to come up with the right higher language and not ask him to
quote function and variable named.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/14] net: sched: use unique idr insert function in unlocked actions

2018-05-20 Thread Marcelo Ricardo Leitner 
On Mon, May 21, 2018 at 12:13:06AM +0300, Or Gerlitz wrote:
> On Sun, May 20, 2018 at 1:20 AM, Marcelo Ricardo Leitner
>  wrote:
> > On Mon, May 14, 2018 at 05:27:14PM +0300, Vlad Buslov wrote:
> >> Substitute calls to action insert function with calls to action insert
> >> unique function that warns if insertion overwrites index in idr.
> >
> > I know this patch may be gone on V2, but a general comment, please use
> > the function names themselves instead of a textualized version. I.e.,
> > s/action insert unique/tcf_idr_insert_unique/
>
> disagree. While doing reviews I found out that if I ask the developer
> to use higher
> level / descriptive language and specifically to avoid putting
> variable / function names in
> patch titles and change logs, the quality gets ++ big time, vs if the
> developer is allowed to say
>
> net/mlx5: Changed add_vovo_bobo()
>
> Added variable do_it_right to add_vovo_bobo(), now we are terribly good.

In your example I agree that it is not helping and it is even allowing
such empty changelog, just as in the section I highlighted, the
descriptive language is also not helping IMHO.

I had to read it 3 times to make sure I wasn't missing a modifier word
when comparing the two functions and well, it's just saying
"Substitute calls to foo function to bar function". I don't see how
the textualized version helps in this case while, at least in this
one, I would have visually recognized the function names way faster.

Sounds like 2 bad examples for either approach.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/14] net: sched: use unique idr insert function in unlocked actions

2018-05-20 Thread Or Gerlitz 
On Sun, May 20, 2018 at 1:20 AM, Marcelo Ricardo Leitner
 wrote:
> On Mon, May 14, 2018 at 05:27:14PM +0300, Vlad Buslov wrote:
>> Substitute calls to action insert function with calls to action insert
>> unique function that warns if insertion overwrites index in idr.
>
> I know this patch may be gone on V2, but a general comment, please use
> the function names themselves instead of a textualized version. I.e.,
> s/action insert unique/tcf_idr_insert_unique/

disagree. While doing reviews I found out that if I ask the developer
to use higher
level / descriptive language and specifically to avoid putting
variable / function names in
patch titles and change logs, the quality gets ++ big time, vs if the
developer is allowed to say

net/mlx5: Changed add_vovo_bobo()

Added variable do_it_right to add_vovo_bobo(), now we are terribly good.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] net: sched: don't release reference on action overwrite

2018-05-20 Thread Vlad Buslov 
On Sat 19 May 2018 at 21:52, Marcelo Ricardo Leitner 
 wrote:
> On Mon, May 14, 2018 at 05:27:10PM +0300, Vlad Buslov wrote:
>> Return from action init function with reference to action taken,
>> even when overwriting existing action.
>
> Isn't this patch necessary before patch 7, to not break things up?
> AFAICU after patchset 7 it assumes the action init function is already
> behaving like this.

I have re-split these patches for V2.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v1] netfilter: provide input interface for route lookup for rpfilter

2018-05-20 Thread Vincent Bernat 
In commit 47b7e7f82802, this bit was removed at the same time the
RT6_LOOKUP_F_IFACE flag was removed. However, it is needed when
link-local addresses are used, which is a very common case: when
packets are routed, neighbor solicitations are done using link-local
addresses. For example, the following neighbor solicitation is not
matched by "-m rpfilter":

IP6 fe80::5254:33ff:fe00:1 > ff02::1:ff00:3: ICMP6, neighbor
solicitation, who has 2001:db8::5254:33ff:fe00:3, length 32

Commit 47b7e7f82802 doesn't quite explain why we shouldn't use
RT6_LOOKUP_F_IFACE in the rpfilter case. I suppose the interface check
later in the function would make it redundant. However, the remaining
of the routing code is using RT6_LOOKUP_F_IFACE when there is no
source address (which matches rpfilter's case with a non-unicast
destination, like with neighbor solicitation).

Signed-off-by: Vincent Bernat 
Fixes: 47b7e7f82802 ("netfilter: don't set F_IFACE on ipv6 fib lookups")
---
 net/ipv6/netfilter/ip6t_rpfilter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c 
b/net/ipv6/netfilter/ip6t_rpfilter.c
index d12f511929f5..0fe61ede77c6 100644
--- a/net/ipv6/netfilter/ip6t_rpfilter.c
+++ b/net/ipv6/netfilter/ip6t_rpfilter.c
@@ -48,6 +48,8 @@ static bool rpfilter_lookup_reverse6(struct net *net, const 
struct sk_buff *skb,
}
 
fl6.flowi6_mark = flags & XT_RPFILTER_VALID_MARK ? skb->mark : 0;
+   if ((flags & XT_RPFILTER_LOOSE) == 0)
+   fl6.flowi6_oif = dev->ifindex;
 
rt = (void *)ip6_route_lookup(net, , skb, lookup_flags);
if (rt->dst.error)
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/14] net: sched: implement reference counted action release

2018-05-20 Thread Vlad Buslov 

On Sun 20 May 2018 at 06:22, Jiri Pirko  wrote:
> Sat, May 19, 2018 at 11:43:27PM CEST, marcelo.leit...@gmail.com wrote:
>>On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote:
>>...
>>> @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct 
>>> nlattr *nla,
>>> return err;
>>>  }
>>>
>>> +static int tcf_action_delete(struct net *net, struct list_head *actions,
>>> +struct netlink_ext_ack *extack)
>>> +{
>>> +   int ret;
>>
>>Reverse christmass tree.. this line should be the last in variable
>>declarations.
>>
>>> +   struct tc_action *a, *tmp;
>>> +   char kind[IFNAMSIZ];
>>> +   u32 act_index;
>>> +
>>> +   list_for_each_entry_safe(a, tmp, actions, list) {
>>> +   const struct tc_action_ops *ops = a->ops;
>>> +
>>> +   /* Actions can be deleted concurrently
>>> +* so we must save their type and id to search again
>>> +* after reference is released.
>>> +*/
>>> +   strncpy(kind, a->ops->kind, sizeof(kind) - 1);
>>
>>This may be problematic. Why strncpy here?
>
> This is not necessary if Vlad is going to hold module referece, ops
> cannot disappear.

Yes, I've already refactored this code to reuse ops.

>
>
>>
>>a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually
>>IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not
>>initialized and strncpy won't add the NULL.
>>
>>> +   act_index = a->tcfa_index;
>>> +
>>> +   list_del(>list);
>>> +   if (tcf_action_put(a))
>>> +   module_put(ops->owner);
>>> +
>>> +   /* now do the delete */
>>> +   ret = tcf_action_del_1(net, kind, act_index, extack);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +   }
>>> +   return 0;
>>> +}

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/14] net: sched: change type of reference and bind counters

2018-05-20 Thread Vlad Buslov 

On Sat 19 May 2018 at 21:04, Marcelo Ricardo Leitner 
 wrote:
> On Mon, May 14, 2018 at 05:27:03PM +0300, Vlad Buslov wrote:
>> Change type of action reference counter to refcount_t.
>>
>> Change type of action bind counter to atomic_t.
>> This type is used to allow decrementing bind counter without testing
>> for 0 result.
>
> ... and in what does not testing for 0 result helps?
>
>   Marcelo

Atomic operations don't WARN in this case.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next,v3 3/3] netfilter: nfnetlink_queue: resolve clash for unconfirmed conntracks

2018-05-20 Thread kbuild test robot 
Hi Pablo,

I love your patch! Yet something to improve:

[auto build test ERROR on nf-next/master]
[also build test ERROR on v4.17-rc5]
[cannot apply to nf/master next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Pablo-Neira-Ayuso/netfilter-add-struct-nf_ct_hook-and-use-it/20180518-093914
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   net//netfilter/nf_conntrack_core.c: In function 'nf_conntrack_update':
>> net//netfilter/nf_conntrack_core.c:1656:36: error: 'struct nf_conn' has no 
>> member named 'zone'
 h = nf_conntrack_find_get(net, >zone, );
   ^~

vim +1656 net//netfilter/nf_conntrack_core.c

  1609  
  1610  static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
  1611  {
  1612  const struct nf_conntrack_l3proto *l3proto;
  1613  const struct nf_conntrack_l4proto *l4proto;
  1614  struct nf_conntrack_tuple_hash *h;
  1615  struct nf_conntrack_tuple tuple;
  1616  enum ip_conntrack_info ctinfo;
  1617  struct nf_nat_hook *nat_hook;
  1618  unsigned int dataoff, status;
  1619  struct nf_conn *ct;
  1620  u16 l3num;
  1621  u8 l4num;
  1622  
  1623  ct = nf_ct_get(skb, );
  1624  if (!ct || nf_ct_is_confirmed(ct))
  1625  return 0;
  1626  
  1627  l3num = nf_ct_l3num(ct);
  1628  l3proto = nf_ct_l3proto_find_get(l3num);
  1629  
  1630  if (l3proto->get_l4proto(skb, skb_network_offset(skb), ,
  1631   ) <= 0)
  1632  return -1;
  1633  
  1634  l4proto = nf_ct_l4proto_find_get(l3num, l4num);
  1635  
  1636  if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, 
l3num,
  1637   l4num, net, , l3proto, l4proto))
  1638  return -1;
  1639  
  1640  if (ct->status & IPS_SRC_NAT) {
  1641  memcpy(tuple.src.u3.all,
  1642 
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.all,
  1643 sizeof(tuple.src.u3.all));
  1644  tuple.src.u.all =
  1645  
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.all;
  1646  }
  1647  
  1648  if (ct->status & IPS_DST_NAT) {
  1649  memcpy(tuple.dst.u3.all,
  1650 
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u3.all,
  1651 sizeof(tuple.dst.u3.all));
  1652  tuple.dst.u.all =
  1653  
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u.all;
  1654  }
  1655  
> 1656  h = nf_conntrack_find_get(net, >zone, );
  1657  if (!h)
  1658  return 0;
  1659  
  1660  /* Store status bits of the conntrack that is clashing to re-do 
NAT
  1661   * mangling according to what it has been done already to this 
packet.
  1662   */
  1663  status = ct->status;
  1664  
  1665  nf_ct_put(ct);
  1666  ct = nf_ct_tuplehash_to_ctrack(h);
  1667  nf_ct_set(skb, ct, ctinfo);
  1668  
  1669  nat_hook = rcu_dereference(nf_nat_hook);
  1670  if (!nat_hook)
  1671  return 0;
  1672  
  1673  if (status & IPS_SRC_NAT &&
  1674  nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_SRC,
  1675  IP_CT_DIR_ORIGINAL) == NF_DROP)
  1676  return -1;
  1677  
  1678  if (status & IPS_DST_NAT &&
  1679  nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_DST,
  1680  IP_CT_DIR_ORIGINAL) == NF_DROP)
  1681  return -1;
  1682  
  1683  return 0;
  1684  }
  1685  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 06/14] net: sched: implement reference counted action release

2018-05-20 Thread Jiri Pirko 
Sat, May 19, 2018 at 11:43:27PM CEST, marcelo.leit...@gmail.com wrote:
>On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote:
>...
>> @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct 
>> nlattr *nla,
>>  return err;
>>  }
>>
>> +static int tcf_action_delete(struct net *net, struct list_head *actions,
>> + struct netlink_ext_ack *extack)
>> +{
>> +int ret;
>
>Reverse christmass tree.. this line should be the last in variable
>declarations.
>
>> +struct tc_action *a, *tmp;
>> +char kind[IFNAMSIZ];
>> +u32 act_index;
>> +
>> +list_for_each_entry_safe(a, tmp, actions, list) {
>> +const struct tc_action_ops *ops = a->ops;
>> +
>> +/* Actions can be deleted concurrently
>> + * so we must save their type and id to search again
>> + * after reference is released.
>> + */
>> +strncpy(kind, a->ops->kind, sizeof(kind) - 1);
>
>This may be problematic. Why strncpy here?

This is not necessary if Vlad is going to hold module referece, ops
cannot disappear.


>
>a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually
>IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not
>initialized and strncpy won't add the NULL.
>
>> +act_index = a->tcfa_index;
>> +
>> +list_del(>list);
>> +if (tcf_action_put(a))
>> +module_put(ops->owner);
>> +
>> +/* now do the delete */
>> +ret = tcf_action_del_1(net, kind, act_index, extack);
>> +if (ret < 0)
>> +return ret;
>> +}
>> +return 0;
>> +}
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html