[PATCH] [rfc, netfilter-next] netfilter: nf_tables: fib warnings

The newly added nft fib code produces two warnings:

net/ipv4/netfilter/nft_fib_ipv4.c: In function 'nft_fib4_eval':
net/ipv4/netfilter/nft_fib_ipv4.c:80:6: error: unused variable 'i' [-Werror=unused-variable]
net/ipv4/netfilter/nft_fib_ipv4.c: In function ‘nft_fib4_eval’:
net/ipv4/netfilter/nft_fib_ipv4.c:137:6: error: ‘oif’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

The first one is obvious as the only user of that variable is
inside of an #ifdef, but the second one is a bit trickier.
It is clear that 'oif' is uninitialized here if neither
NFTA_FIB_F_OIF nor NFTA_FIB_F_IIF are set.

I have no idea how that should be handled, this patch just
returns without doing anything, which may or may not be
the right thing to do.

Fixes: 84f5eedb983e ("netfilter: nf_tables: add fib expression")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 net/ipv4/netfilter/nft_fib_ipv4.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
index 6787c563cfc9..b29f70593e8b 100644
--- a/net/ipv4/netfilter/nft_fib_ipv4.c
+++ b/net/ipv4/netfilter/nft_fib_ipv4.c

@@ -77,7 +77,9 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	};
 	const struct net_device *oif;
 	struct net_device *found;
+#ifdef CONFIG_IP_ROUTE_MULTIPATH
 	int i;
+#endif
 
 	/*
 	 * Do not set flowi4_oif, it restricts results (for example, asking

@@ -90,6 +92,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
 		oif = pkt->out;
 	else if (priv->flags & NFTA_FIB_F_IIF)
 		oif = pkt->in;
+	else
+		return;
 
 	if (pkt->hook == NF_INET_PRE_ROUTING && fib4_is_local(pkt->skb)) {
 		nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX);

-- 
2.9.0

Arnd Bergmann [off-list ref] wrote:

The newly added nft fib code produces two warnings:

net/ipv4/netfilter/nft_fib_ipv4.c: In function 'nft_fib4_eval':
net/ipv4/netfilter/nft_fib_ipv4.c:80:6: error: unused variable 'i' [-Werror=unused-variable]
net/ipv4/netfilter/nft_fib_ipv4.c: In function ‘nft_fib4_eval’:
net/ipv4/netfilter/nft_fib_ipv4.c:137:6: error: ‘oif’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

The first one is obvious as the only user of that variable is
inside of an #ifdef, but the second one is a bit trickier.
It is clear that 'oif' is uninitialized here if neither
NFTA_FIB_F_OIF nor NFTA_FIB_F_IIF are set.

I have no idea how that should be handled, this patch just
returns without doing anything, which may or may not be
the right thing to do.

It should be initialized to NULL.

On Friday, October 28, 2016 5:50:31 PM CEST Florian Westphal wrote:

Arnd Bergmann [off-list ref] wrote:

quoted

The newly added nft fib code produces two warnings:

net/ipv4/netfilter/nft_fib_ipv4.c: In function 'nft_fib4_eval':
net/ipv4/netfilter/nft_fib_ipv4.c:80:6: error: unused variable 'i' [-Werror=unused-variable]
net/ipv4/netfilter/nft_fib_ipv4.c: In function ‘nft_fib4_eval’:
net/ipv4/netfilter/nft_fib_ipv4.c:137:6: error: ‘oif’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

The first one is obvious as the only user of that variable is
inside of an #ifdef, but the second one is a bit trickier.
It is clear that 'oif' is uninitialized here if neither
NFTA_FIB_F_OIF nor NFTA_FIB_F_IIF are set.

I have no idea how that should be handled, this patch just
returns without doing anything, which may or may not be
the right thing to do.

It should be initialized to NULL.

Ok, I had considered that, but wasn't sure if ->nh_dev could
ever be NULL, as that would then get dereferenced.

	Arnd

Arnd Bergmann [off-list ref] wrote:

On Friday, October 28, 2016 5:50:31 PM CEST Florian Westphal wrote:

quoted

Arnd Bergmann [off-list ref] wrote:

quoted

The newly added nft fib code produces two warnings:

net/ipv4/netfilter/nft_fib_ipv4.c: In function 'nft_fib4_eval':
net/ipv4/netfilter/nft_fib_ipv4.c:80:6: error: unused variable 'i' [-Werror=unused-variable]
net/ipv4/netfilter/nft_fib_ipv4.c: In function ‘nft_fib4_eval’:
net/ipv4/netfilter/nft_fib_ipv4.c:137:6: error: ‘oif’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

The first one is obvious as the only user of that variable is
inside of an #ifdef, but the second one is a bit trickier.
It is clear that 'oif' is uninitialized here if neither
NFTA_FIB_F_OIF nor NFTA_FIB_F_IIF are set.

I have no idea how that should be handled, this patch just
returns without doing anything, which may or may not be
the right thing to do.

It should be initialized to NULL.

Ok, I had considered that, but wasn't sure if ->nh_dev could
ever be NULL, as that would then get dereferenced.

Good point.  In case oif is NULL we don't have to search the result
list for a match anyway, so we could do this (not even build tested):

diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
--- a/net/ipv4/netfilter/nft_fib_ipv4.c
+++ b/net/ipv4/netfilter/nft_fib_ipv4.c

@@ -130,6 +130,11 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
                break;
        }
 
+       if (!oif) {
+               found = FIB_RES_DEV(res);
+               goto ok;
+       }
+
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
        for (i = 0; i < res.fi->fib_nhs; i++) {
                struct fib_nh *nh = &res.fi->fib_nh[i];

@@ -139,16 +144,12 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
                        goto ok;
                }
        }
-#endif
-       if (priv->flags & NFTA_FIB_F_OIF) {
-               found = FIB_RES_DEV(res);
-               if (found == oif)
-                       goto ok;
-               return;
-       }
-
-       *dest = FIB_RES_DEV(res)->ifindex;
        return;
+#else
+       found = FIB_RES_DEV(res);
+       if (found != oif)
+               return;
+#endif
 ok:
        switch (priv->result) {

I can take care of this as a followup.

On Friday, October 28, 2016 6:21:49 PM CEST Florian Westphal wrote:

Good point.  In case oif is NULL we don't have to search the result
list for a match anyway, so we could do this (not even build tested):

It didn't apply cleanly, but I've integrated it with the change to
initialize oif to NULL and the added #ifdef I had in my first version
and got a clean build.

I sent out a v2 now, but didn't try hard to understand your changes.

	Arnd