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

[Arnd Bergmann]

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

--
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] [rfc, netfilter-next] netfilter: nf_tables: fib warnings

[Florian Westphal]

Arnd Bergmann  wrote:
> On Friday, October 28, 2016 5:50:31 PM CEST Florian Westphal wrote:
> > Arnd Bergmann  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.
> 
> 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 = >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.

--
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] [rfc, netfilter-next] netfilter: nf_tables: fib warnings

[Arnd Bergmann]

On Friday, October 28, 2016 5:50:31 PM CEST Florian Westphal wrote:
> Arnd Bergmann  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.

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

Arnd

--
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] [rfc, netfilter-next] netfilter: nf_tables: fib warnings

[Florian Westphal]

Arnd Bergmann  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.
--
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] [rfc, netfilter-next] netfilter: nf_tables: fib warnings

[Arnd Bergmann]

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 
---
 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

--
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