* Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location
From: pravin shelar @ 2016-09-30 18:20 UTC (permalink / raw)
To: Jiri Benc; +Cc: Linux Kernel Network Developers, David Ahern
In-Reply-To: <20160930195159.5b7eed00@griffin>
On Fri, Sep 30, 2016 at 10:51 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Fri, 30 Sep 2016 10:43:42 -0700, pravin shelar wrote:
>> There is already struct mpls_label defined in uapi mpls header file.
>> Is there reason for not using it here?
>
> I think this should be unified as well. But it's for a different patch
> not related to this patchset. This patch just moves the existing
> mpls_hdr and mpls_shim_hdr definition to a new place.
>
> In general, I think the mpls code deserves a round of overall cleanups.
> It should not block this patchset, though. It can be considered as a
> step towards that goal.
>
Alright, I do not oppose this patch going in as it is.
Acked-by: Pravin B Shelar <pshelar@ovn.org>
^ permalink raw reply
* Re: [PATCH nf-next v4 1/2] netfilter: Fix potential null pointer dereference
From: Pablo Neira Ayuso @ 2016-09-30 17:41 UTC (permalink / raw)
To: Aaron Conole; +Cc: netfilter-devel, netdev, Florian Westphal
In-Reply-To: <1475076915-5920-2-git-send-email-aconole@bytheb.org>
On Wed, Sep 28, 2016 at 11:35:14AM -0400, Aaron Conole wrote:
> It's possible for nf_hook_entry_head to return NULL. If two
> nf_unregister_net_hook calls happen simultaneously with a single hook
> entry in the list, both will enter the nf_hook_mutex critical section.
> The first will successfully delete the head, but the second will see
> this NULL pointer and attempt to dereference.
>
> This fix ensures that no null pointer dereference could occur when such
> a condition happens.
Applied, thanks Aaron.
^ permalink raw reply
* Re: [PATCH 2/2] mlxsw: spectrum_router: avoid potential uninitialized data usage
From: Ido Schimmel @ 2016-09-30 17:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jiri Pirko, Ido Schimmel, David S. Miller, Yotam Gigi,
Nogah Frankel, netdev, linux-kernel
In-Reply-To: <20160930161734.70928-2-arnd@arndb.de>
On Fri, Sep 30, 2016 at 06:17:10PM +0200, Arnd Bergmann wrote:
> If fi->fib_nhs is zero, the router interface pointer is uninitialized, as shown by
> this warning:
>
> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c: In function 'mlxsw_sp_router_fib_event':
> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c:1674:21: error: 'r' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c:1643:23: note: 'r' was declared here
>
> This changes the loop so we handle the case the same way as finding no router
> interface pointer attached to one of the nexthops to ensure we always
> trap here instead of using uninitialized data.
>
> Fixes: b45f64d16d45 ("mlxsw: spectrum_router: Use FIB notifications instead of switchdev calls")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For net-next:
Acked-by: Ido Schimmel <idosch@mellanox.com>
BTW, which flags did you pass to generate this error? I can only
reproduce this with EXTRA_CFLAGS="-Wmaybe-uninitialized -Werror", but
for some reason EXTRA_CFLAGS="-Wall -Werror" is silent. Any idea why? I
would like to add this to my git hooks and avoid these errors in the
future :)
Thanks!
^ permalink raw reply
* Re: [PATCH net-next v3] netfilter: xt_hashlimit: Fix link error in 32bit arch because of 64bit division
From: Vishwanath Pai @ 2016-09-30 17:58 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: johunt, netfilter-devel, coreteam, netdev, pai.vishwain, kaber,
kadlec, zlpnobody, hannes, maze, eric.dumazet, akpm
In-Reply-To: <20160930174650.GE1176@salvia>
On 09/30/2016 01:46 PM, Pablo Neira Ayuso wrote:
> On Thu, Sep 29, 2016 at 01:39:50PM -0400, Vishwanath Pai wrote:
>> v2:
>> Remove unnecessary div64_u64 around constants
>>
>> v3:
>> remove backslashes
>>
>> --
>>
>> Fix link error in 32bit arch because of 64bit division
>>
>> Division of 64bit integers will cause linker error undefined reference
>> to `__udivdi3'. Fix this by replacing divisions with div64_64
>
> Applied, thanks Pai.
>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to ...")
>>
>> ---
>
> Please, next time place the versioning information here, otherwise git
> am takes the wrong description here.
>
Thank you. And sorry about, I did not realize git am would put that into
the description.
-Vishwanath
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location
From: Jiri Benc @ 2016-09-30 17:51 UTC (permalink / raw)
To: pravin shelar; +Cc: Linux Kernel Network Developers, David Ahern
In-Reply-To: <CAOrHB_BJqka5HGpeJ1GF42X2kQOmzrLY_LQt4Td6zrAX84Fjow@mail.gmail.com>
On Fri, 30 Sep 2016 10:43:42 -0700, pravin shelar wrote:
> There is already struct mpls_label defined in uapi mpls header file.
> Is there reason for not using it here?
I think this should be unified as well. But it's for a different patch
not related to this patchset. This patch just moves the existing
mpls_hdr and mpls_shim_hdr definition to a new place.
In general, I think the mpls code deserves a round of overall cleanups.
It should not block this patchset, though. It can be considered as a
step towards that goal.
Jiri
^ permalink raw reply
* Re: [PATCH 1/3] netfilter: nf_tables: avoid uninitialized variable warning
From: Pablo Neira Ayuso @ 2016-09-30 17:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Patrick McHardy, Jozsef Kadlecsik, David S. Miller,
netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <20160930160559.4102745-1-arnd@arndb.de>
On Fri, Sep 30, 2016 at 06:05:34PM +0200, Arnd Bergmann wrote:
> The newly added nft_range_eval() function handles the two possible
> nft range operations, but as the compiler warning points out,
> any unexpected value would lead to the 'mismatch' variable being
> used without being initialized:
>
> net/netfilter/nft_range.c: In function 'nft_range_eval':
> net/netfilter/nft_range.c:45:5: error: 'mismatch' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This can be trivially avoided by added a 'default:' clause.
Applied this patch, I took Aaron's and Pai's patches instead.
Thanks anyway for following up on this issue Arnd.
^ permalink raw reply
* Re: [PATCH net-next v3] netfilter: xt_hashlimit: Fix link error in 32bit arch because of 64bit division
From: Pablo Neira Ayuso @ 2016-09-30 17:46 UTC (permalink / raw)
To: Vishwanath Pai
Cc: johunt, netfilter-devel, coreteam, netdev, pai.vishwain, kaber,
kadlec, zlpnobody, hannes, maze, eric.dumazet, akpm
In-Reply-To: <20160929173950.GA27689@akamai.com>
On Thu, Sep 29, 2016 at 01:39:50PM -0400, Vishwanath Pai wrote:
> v2:
> Remove unnecessary div64_u64 around constants
>
> v3:
> remove backslashes
>
> --
>
> Fix link error in 32bit arch because of 64bit division
>
> Division of 64bit integers will cause linker error undefined reference
> to `__udivdi3'. Fix this by replacing divisions with div64_64
Applied, thanks Pai.
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to ...")
>
> ---
Please, next time place the versioning information here, otherwise git
am takes the wrong description here.
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location
From: pravin shelar @ 2016-09-30 17:43 UTC (permalink / raw)
To: Jiri Benc; +Cc: Linux Kernel Network Developers, David Ahern
In-Reply-To: <4556158d9b279156a5c41054f1e803c004bc28e9.1475255100.git.jbenc@redhat.com>
On Fri, Sep 30, 2016 at 10:08 AM, Jiri Benc <jbenc@redhat.com> wrote:
> This will be also used by openvswitch.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> v2: new in v2
> ---
> include/net/mpls.h | 9 +++++++++
> net/mpls/internal.h | 10 +---------
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/mpls.h b/include/net/mpls.h
> index 5b3b5addfb08..3ebbc0bb57ff 100644
> --- a/include/net/mpls.h
> +++ b/include/net/mpls.h
> @@ -19,12 +19,21 @@
>
> #define MPLS_HLEN 4
>
> +struct mpls_shim_hdr {
> + __be32 label_stack_entry;
> +};
> +
There is already struct mpls_label defined in uapi mpls header file.
Is there reason for not using it here?
^ permalink raw reply
* Re: [PATCH nf-next v4 2/2] nf_set_hooks_head: accommodate different kconfig
From: Pablo Neira Ayuso @ 2016-09-30 17:43 UTC (permalink / raw)
To: Aaron Conole; +Cc: netfilter-devel, netdev, Florian Westphal
In-Reply-To: <1475076915-5920-3-git-send-email-aconole@bytheb.org>
On Wed, Sep 28, 2016 at 11:35:15AM -0400, Aaron Conole wrote:
> When CONFIG_NETFILTER_INGRESS is unset (or no), we need to handle
> the request for registration properly by dropping the hook. This
> releases the entry during the set.
Also applied.
I have renamed the subject to:
netfilter: accommodate different kconfig in nf_set_hooks_head()
to make it look similar to what we have in the history.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] netfilter: xt_hashlimit: uses div_u64 for division
From: Arnd Bergmann @ 2016-09-30 17:39 UTC (permalink / raw)
To: Eric Dumazet
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Joshua Hunt, Vishwanath Pai, netfilter-devel,
coreteam, netdev, linux-kernel
In-Reply-To: <1475253522.28155.195.camel@edumazet-glaptop3.roam.corp.google.com>
On Friday 30 September 2016, Eric Dumazet wrote:
> On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote:
> > net/netfilter/xt_hashlimit.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index 44a095ecc7b7..3d5525df6eb3 100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len)
> > static u64 user2credits(u64 user, int revision)
> > {
> > if (revision == 1) {
> > + u32 user32 = user; /* use 32-bit division */
> > +
>
> This looks dangerous to me. Have you really tried all possible cases ?
Yes, I'm pretty certain about that: The 11d5f15723c9 patch that introduced this
kept the existing implementation for the revision==1 case, except for changing
the types.
> Caller (even if using revision == 1) does
> user2credits(cfg->avg * cfg->burst, revision);
>
> Since this is not a fast path, I would prefer to keep the 64bit divide.
>
> Vishwanath version looks safer.
Ok, fair enough. I couldn't tell how much of a fast path this
was, and it's more a general issue that I see with other developers
blindly using div_u64() whenever getting this link error.
Since I already had the patch by the time I saw the other one
(which is also at v3 and got comments), I just sent it out along
with the other two patches I had for netfilter.
I also ended up introducing a typo in a last-minute change, so I'll let
Vishwanath and you work out the best implementation and withdraw my
version.
Arnd
^ permalink raw reply
* Re: UDP wierdness around skb_copy_and_csum_datagram_msg()
From: Jay Smith @ 2016-09-30 17:35 UTC (permalink / raw)
To: Christian Lamparter
Cc: Jay Smith, Alan Curry, Eric Dumazet, netdev, Al Viro,
davem@davemloft.net
In-Reply-To: <3897844.9kvF4Wy8aS@debian64>
Christian Lamparter writes:
> On Wednesday, September 28, 2016 7:20:39 PM CEST Jay Smith wrote:
>> Actually, on a little more searching of this list's archives, I think
>> that this discussion: https://patchwork.kernel.org/patch/9260733/ is
>> about exactly the same issue I've found, except from the TCP side. I'm
>> cc'ing a few of the participants from that discussion.
>>
>> So is the patch proposed there (copying and restoring the entire
>> iov_iter in skb_copy_and_csum_datagram_msg()) being considered as a
>> fix?
>
> From Alan's post:
>
> "My ugly patch fixes this in the most obvious way: make a local copy of
> msg->msg_iter before the call to skb_copy_and_csum_datagram(), and copy
> it back if the checksum is bad, just before goto csum_error;"
>
> IMHO this meant that the patch is a proof of concept for his problem.
It's also the simplest thing that fixes all of the relevant cases (udp4,
udp6, tcp4). Basically, the state of the iov_iter (which, if I'm
reading correctly, consists of three elements -- iov_offset, count, and
nr_segs all change values as the iterator moves through the vectors)
needs to be backed-up and restored at exactly the points in datagram.c
where Alan's patch does so.
Whether that should be done with memcpy, as Alan does, or by exposing
some more abstract backup/restore functions from iov_iter.c is a matter
of taste. I'm happy to accept the call of someone more maintainer-ish
on that.
> Al Viro identified more inconsistencies within the error-paths that deal
> with EFAULT in the whole area (in and around skb_copy_and_csum_datagram()).
Was this in some other thread? The only other discussion I see of that
function in the "PROBLEM: network data corruption..." thread is around
this patch https://patchwork.kernel.org/patch/9245023/ , which as Al
says was just a diagnostic patch -- it intentionally doesn't handle the
multiple-vector case.
It seems like the EFAULT case in skb_copy_and_csum_datagram() would
indicate that the iov_iter code ran out of room to copy the current
message, even though it's checked for that room at datagram.c:738.
Which I guess is possible -- there could be some non-obvious counting
error in the iov_inter.c macros. But, at least in the UDP cases, it
wouldn't trigger the same problem as a checksum failure -- the EFAULT
gets returned to the caller in that case, and the buffer isn't meant
to be valid. It's only in the checksum case that we retry underneath
the udp_recvmsg() covers, and end up returning the supposedly-rejected data.
>
> As for fixing the issue: I'm happy to test and review patches.
> The trouble is that nobody seem to be able to produce them...
Sorry -- is the trouble you're talking about here that no-one's produced
a patch, or that we don't have a reproduction of the problem? I don't
think either is true.
The test program I'd attached to my first mail reliably reproduces
the UDP version of the problem. It's pretty simple: listen on a UDP
port (using loopback, so that there's no hardware csum offload), use a
raw socket to send a datagram with a bad UDP checksum, then send a good
datagram, and then finally read from the socket. On post-3.19 kernels,
you always get the contents of the bad packet at the start of the user
buffer:
# bin/csumtestn 69
listening on port 47193
recvmsg returned 9 bytes: BAD DATABAD DATABAD DATABAD DATABAD DATABAD DATABAD DATABAD DATABAD DGood data
After Alan's patch, the good packet's contents are at the start of the
buffer, where they belong:
# bin/csumtestn 69
listening on port 54620
recvmsg returned 9 bytes: Good dataAD DATABAD DATABAD DATABAD DATABAD DATABAD DATABAD DATABAD D
So functionally, I believe that Alan's patch does the trick. I haven't
actually tested it on UDP6, but a similar test should work there.
Inserting the bad packets deterministically into a TCP connection is
trickier, but I thought in the previous thread that you and Alan both
had wireless hardware configurations that frequently generated checksum
errors, and that Alan's claim was that his patch gave him good TCP data
even in the presence of those checksum errors. Or do I misunderstand?
(Just to be clear, though, if there is a need for a new patch, for
whatever reason, I'm happy to generate one.)
^ permalink raw reply
* Re: [PATCH 3/3] netfilter: xt_hashlimit: uses div_u64 for division
From: Vishwanath Pai @ 2016-09-30 17:21 UTC (permalink / raw)
To: Eric Dumazet, Arnd Bergmann
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Joshua Hunt, netfilter-devel, coreteam, netdev,
linux-kernel
In-Reply-To: <1475253522.28155.195.camel@edumazet-glaptop3.roam.corp.google.com>
On 09/30/2016 12:38 PM, Eric Dumazet wrote:
> On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote:
>> The newly added support for high-resolution pps rates introduced multiple 64-bit
>> division operations in one function, which fails on all 32-bit architectures:
>>
>> net/netfilter/xt_hashlimit.o: In function `user2credits':
>> xt_hashlimit.c:(.text.user2credits+0x3c): undefined reference to `__aeabi_uldivmod'
>> xt_hashlimit.c:(.text.user2credits+0x68): undefined reference to `__aeabi_uldivmod'
>> xt_hashlimit.c:(.text.user2credits+0x88): undefined reference to `__aeabi_uldivmod'
>>
>> This replaces the division with an explicit call to div_u64 for version 2
>> to documents that this is a slow operation, and reverts back to 32-bit arguments
>> for the version 1 data to restore the original faster 32-bit division.
>>
>> With both changes combined, we no longer get a link error.
>>
>> Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to support higher pps rates")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> Vishwanath Pai already sent a patch for this, and I did my version independently.
>> The difference is that his version also the more expensive division for the
>> version 1 variant that doesn't need it.
>>
>> See also http://patchwork.ozlabs.org/patch/676713/
>> ---
>> net/netfilter/xt_hashlimit.c | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> index 44a095ecc7b7..3d5525df6eb3 100644
>> --- a/net/netfilter/xt_hashlimit.c
>> +++ b/net/netfilter/xt_hashlimit.c
>> @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len)
>> static u64 user2credits(u64 user, int revision)
>> {
>> if (revision == 1) {
>> + u32 user32 = user; /* use 32-bit division */
>> +
>
> This looks dangerous to me. Have you really tried all possible cases ?
>
> Caller (even if using revision == 1) does
> user2credits(cfg->avg * cfg->burst, revision);
>
It does look like we might lose precision here because of 64bit to 32bit
conversion, but I am not sure how much it matters here. Iirc this is how
it used to be before rev2 code.
> Since this is not a fast path, I would prefer to keep the 64bit divide.
>
Agreed, this code does not get executed too often for us to worry about
div_u64 being slow. And it reverts back to regular division on 64 bit
arch anyways.
> Vishwanath version looks safer.
>
>
-Vishwanath
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location
From: David Ahern @ 2016-09-30 17:14 UTC (permalink / raw)
To: Jiri Benc, netdev; +Cc: pravin shelar
In-Reply-To: <4556158d9b279156a5c41054f1e803c004bc28e9.1475255100.git.jbenc@redhat.com>
On 9/30/16 11:08 AM, Jiri Benc wrote:
> This will be also used by openvswitch.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> v2: new in v2
> ---
> include/net/mpls.h | 9 +++++++++
> net/mpls/internal.h | 10 +---------
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
Acked-by: David Ahern <dsa@cumulusnetworks.com>
^ permalink raw reply
* [PATCH net-next v2 1/3] openvswitch: mpls: set network header correctly on key extract
From: Jiri Benc @ 2016-09-30 17:08 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, pravin shelar
In-Reply-To: <cover.1475255100.git.jbenc@redhat.com>
After the 48d2ab609b6b ("net: mpls: Fixups for GSO"), MPLS handling in
openvswitch was changed to have network header pointing to the start of the
MPLS headers and inner_network_header pointing after the MPLS headers.
However, key_extract was missed by the mentioned commit, causing incorrect
headers to be set when a MPLS packet just enters the bridge or after it is
recirculated.
Fixes: 48d2ab609b6b ("net: mpls: Fixups for GSO")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
Acked-by: Pravin B Shelar <pshelar@ovn.org>
---
v2: no changes
---
net/openvswitch/flow.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 634cc10d6dee..c8c82e109c68 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -633,12 +633,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
} else if (eth_p_mpls(key->eth.type)) {
size_t stack_len = MPLS_HLEN;
- /* In the presence of an MPLS label stack the end of the L2
- * header and the beginning of the L3 header differ.
- *
- * Advance network_header to the beginning of the L3
- * header. mac_len corresponds to the end of the L2 header.
- */
+ skb_set_inner_network_header(skb, skb->mac_len);
while (1) {
__be32 lse;
@@ -646,12 +641,12 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
if (unlikely(error))
return 0;
- memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
+ memcpy(&lse, skb_inner_network_header(skb), MPLS_HLEN);
if (stack_len == MPLS_HLEN)
memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
- skb_set_network_header(skb, skb->mac_len + stack_len);
+ skb_set_inner_network_header(skb, skb->mac_len + stack_len);
if (lse & htonl(MPLS_LS_S_MASK))
break;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v2 3/3] openvswitch: use mpls_hdr
From: Jiri Benc @ 2016-09-30 17:08 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, pravin shelar
In-Reply-To: <cover.1475255100.git.jbenc@redhat.com>
skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
instead.
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
v2: using the existing mpls_hdr instead of skb_network_header
---
include/net/mpls.h | 12 ------------
net/openvswitch/actions.c | 24 ++++++++++++------------
2 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/include/net/mpls.h b/include/net/mpls.h
index 3ebbc0bb57ff..1dbc669b770e 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -33,16 +33,4 @@ static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
{
return (struct mpls_shim_hdr *)skb_network_header(skb);
}
-
-/*
- * For non-MPLS skbs this will correspond to the network header.
- * For MPLS skbs it will be before the network_header as the MPLS
- * label stack lies between the end of the mac header and the network
- * header. That is, for MPLS skbs the end of the mac header
- * is the top of the MPLS label stack.
- */
-static inline unsigned char *skb_mpls_header(struct sk_buff *skb)
-{
- return skb_mac_header(skb) + skb->mac_len;
-}
#endif
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 863e992dfbc0..4e03f64709bc 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -160,7 +160,7 @@ static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
const struct ovs_action_push_mpls *mpls)
{
- __be32 *new_mpls_lse;
+ struct mpls_shim_hdr *new_mpls_lse;
/* Networking stack do not allow simultaneous Tunnel and MPLS GSO. */
if (skb->encapsulation)
@@ -180,8 +180,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb->mac_len);
- new_mpls_lse = (__be32 *)skb_mpls_header(skb);
- *new_mpls_lse = mpls->mpls_lse;
+ new_mpls_lse = mpls_hdr(skb);
+ new_mpls_lse->label_stack_entry = mpls->mpls_lse;
skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
@@ -202,7 +202,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
if (unlikely(err))
return err;
- skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
+ skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
skb->mac_len);
@@ -211,10 +211,10 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb->mac_len);
- /* skb_mpls_header() is used to locate the ethertype
- * field correctly in the presence of VLAN tags.
+ /* mpls_hdr() is used to locate the ethertype field correctly in the
+ * presence of VLAN tags.
*/
- hdr = (struct ethhdr *)(skb_mpls_header(skb) - ETH_HLEN);
+ hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
update_ethertype(skb, hdr, ethertype);
if (eth_p_mpls(skb->protocol))
skb->protocol = ethertype;
@@ -226,7 +226,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
const __be32 *mpls_lse, const __be32 *mask)
{
- __be32 *stack;
+ struct mpls_shim_hdr *stack;
__be32 lse;
int err;
@@ -234,16 +234,16 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
if (unlikely(err))
return err;
- stack = (__be32 *)skb_mpls_header(skb);
- lse = OVS_MASKED(*stack, *mpls_lse, *mask);
+ stack = mpls_hdr(skb);
+ lse = OVS_MASKED(stack->label_stack_entry, *mpls_lse, *mask);
if (skb->ip_summed == CHECKSUM_COMPLETE) {
- __be32 diff[] = { ~(*stack), lse };
+ __be32 diff[] = { ~(stack->label_stack_entry), lse };
skb->csum = ~csum_partial((char *)diff, sizeof(diff),
~skb->csum);
}
- *stack = lse;
+ stack->label_stack_entry = lse;
flow_key->mpls.top_lse = lse;
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location
From: Jiri Benc @ 2016-09-30 17:08 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, pravin shelar
In-Reply-To: <cover.1475255100.git.jbenc@redhat.com>
This will be also used by openvswitch.
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
v2: new in v2
---
include/net/mpls.h | 9 +++++++++
net/mpls/internal.h | 10 +---------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/net/mpls.h b/include/net/mpls.h
index 5b3b5addfb08..3ebbc0bb57ff 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -19,12 +19,21 @@
#define MPLS_HLEN 4
+struct mpls_shim_hdr {
+ __be32 label_stack_entry;
+};
+
static inline bool eth_p_mpls(__be16 eth_type)
{
return eth_type == htons(ETH_P_MPLS_UC) ||
eth_type == htons(ETH_P_MPLS_MC);
}
+static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
+{
+ return (struct mpls_shim_hdr *)skb_network_header(skb);
+}
+
/*
* For non-MPLS skbs this will correspond to the network header.
* For MPLS skbs it will be before the network_header as the MPLS
diff --git a/net/mpls/internal.h b/net/mpls/internal.h
index 732a5c17e986..bdfef6c3271a 100644
--- a/net/mpls/internal.h
+++ b/net/mpls/internal.h
@@ -1,9 +1,6 @@
#ifndef MPLS_INTERNAL_H
#define MPLS_INTERNAL_H
-
-struct mpls_shim_hdr {
- __be32 label_stack_entry;
-};
+#include <net/mpls.h>
struct mpls_entry_decoded {
u32 label;
@@ -93,11 +90,6 @@ struct mpls_route { /* next hop label forwarding entry */
#define endfor_nexthops(rt) }
-static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
-{
- return (struct mpls_shim_hdr *)skb_network_header(skb);
-}
-
static inline struct mpls_shim_hdr mpls_entry_encode(u32 label, unsigned ttl, unsigned tc, bool bos)
{
struct mpls_shim_hdr result;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v2 0/3] openvswitch: mpls fix and clean up
From: Jiri Benc @ 2016-09-30 17:08 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, pravin shelar
Convert to the new mpls skb layout the last remaining place in openvswitch,
forgotten on the mpls GSO rework. The GSO rework also allows for some
cleanup in the third patch.
Jiri Benc (3):
openvswitch: mpls: set network header correctly on key extract
mpls: move mpls_hdr to a common location
openvswitch: use mpls_hdr
include/net/mpls.h | 15 ++++++---------
net/mpls/internal.h | 10 +---------
net/openvswitch/actions.c | 24 ++++++++++++------------
net/openvswitch/flow.c | 11 +++--------
4 files changed, 22 insertions(+), 38 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH 2/3] netfilter: hide reference to nf_hooks_ingress
From: Aaron Conole @ 2016-09-30 17:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Florian Westphal, netfilter-devel, coreteam,
netdev, linux-kernel
In-Reply-To: <20160930160559.4102745-2-arnd@arndb.de>
Arnd Bergmann <arnd@arndb.de> writes:
> A recent cleanup added an unconditional reference to the nf_hooks_ingress pointer,
> but that fails when CONFIG_NETFILTER_INGRESS is disabled and that member is
> not present in net_device:
>
> net/netfilter/core.c: In function 'nf_set_hooks_head':
> net/netfilter/core.c:96:30: error: 'struct net_device' has no member named 'nf_hooks_ingress'
>
> This avoids the build error by simply enclosing the assignment in an #ifdef,
> which may or may not be the correct fix.
NAK, it's not the right fix. The entry being set may be leaked with only this
hunk. I've posted a complete fix for this.
Sorry that it was broken.
^ permalink raw reply
* Re: [PATCH RFC iproute2] iplink: Support envhdrlen
From: Toshiaki Makita @ 2016-09-30 16:49 UTC (permalink / raw)
To: Jiri Benc, Toshiaki Makita
Cc: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
Jeff Kirsher
In-Reply-To: <20160929144944.609d5434@griffin>
On 16/09/29 (木) 21:49, Jiri Benc wrote:
> On Tue, 27 Sep 2016 17:55:40 +0900, Toshiaki Makita wrote:
>> This adds support for envhdrlen.
>>
>> Example:
>> # ip link set eno1 envhdrlen 8
Thank you for taking a look at this.
> I don't see why this should be user visible, let alone requiring user
> to set it. This should be transparent, kernel should compute the value
> as needed based on the configuration and set it up. Requiring the
> administrator to pick up a calculator and sum up all the vlan, mpls and
> whatever header lengths is silly.
I'm thinking both in-kernel automation (for VLAN) and users' manual
operation are necessary. In fact, as I stated in the cover letter, my
first implementation was an automation-based approach. Actually
automation is advanced form of this feature so I'm proposing this very
basic feature first.
> I realize that we currently have no easy way to do that. Especially
> with lwtunnels and stuff line MPLS where we don't easily know the
> number of tags. But every uAPI we introduce will have to be supported
> forever and going a particular way just because it is easy to implement
> is not sustainable.
It may be possible for MPLS lwtunnel to notify underlying device of
needed header length. But at this point MPLS does not allow encapsulated
packets to be greater than MTU of underlying device. So how to determine
if someone wants to leverage envhdrlen instead? Add a knob to lwtunnel
layer? Then, add another knob to l2tp, iptunnel, or anything like that?
At least full-automation does not look possible other than VLAN (which
by default requires envhdrlen expansion), so anyway manual operation is
needed in some form.
Another use-case is reducing envhdrlen. We can expand it on creating
VLAN device automatically, but cannot decrease the size because there
could be other consumers of envhdrlen.
> At the very least, it should be configurable from the other direction.
> I.e. telling which interfaces can be used by vlans or MPLS (if it
> cannot be inferred automatically) and configuring maximum number of
> tags on the given vlan/mpls/whatever interface/route/whatever.
Probably I don't get your point...
Are you suggesting something like this?
$ ip link set eth0.10.20 expand-realdev-envhdrlen
or like this?
$ ip link set eth0 allowed-vlan-tags 2
Toshiaki Makita
^ permalink raw reply
* [net-next PATCH v2] i40e: Clean up handling of msglevel flags and debug parameter
From: Alexander Duyck @ 2016-09-30 10:42 UTC (permalink / raw)
To: sassmann, intel-wired-lan, jeffrey.t.kirsher, netdev; +Cc: davem, linville
So the i40e driver had a really convoluted configuration for how to handle
the debug flags contained in msg_level. Part of the issue is that the
driver has its own 32 bit mask that it was using to track a separate set of
debug features. From what I can tell it was trying to use the upper 4 bits
to determine if the value was meant to represent a bit-mask or the numeric
value provided by debug level.
What this patch does is clean this up by compressing those 4 bits into bit
31, as a result we just have to perform a check against the value being
negative to determine if we are looking at a debug level (positive), or a
debug mask (negative). The debug level will populate the msg_level, and
the debug mask will populate the debug_mask in the hardware struct.
I added similar logic for ethtool. If the value being provided has bit 31
set we assume the value being provided is a debug mask, otherwise we assume
it is a msg_enable mask. For displaying we only provide the msg_enable,
and if debug_mask is in use we will print it to the dmesg log.
Lastly I removed the debugfs interface. It is redundant with what we
already have in ethtool and really doesn't belong anyway.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
v2: Consolidated setup for msg_enable and hw.debug_mask to one point in probe.
drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 18 ------------------
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 ++++++-
drivers/net/ethernet/intel/i40e/i40e_main.c | 23 ++++++++---------------
3 files changed, 14 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 0c1875b..acb0f13 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -1210,24 +1210,6 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
dev_info(&pf->pdev->dev,
"dump debug fwdata <cluster_id> <table_id> <index>\n");
}
-
- } else if (strncmp(cmd_buf, "msg_enable", 10) == 0) {
- u32 level;
- cnt = sscanf(&cmd_buf[10], "%i", &level);
- if (cnt) {
- if (I40E_DEBUG_USER & level) {
- pf->hw.debug_mask = level;
- dev_info(&pf->pdev->dev,
- "set hw.debug_mask = 0x%08x\n",
- pf->hw.debug_mask);
- }
- pf->msg_enable = level;
- dev_info(&pf->pdev->dev, "set msg_enable = 0x%08x\n",
- pf->msg_enable);
- } else {
- dev_info(&pf->pdev->dev, "msg_enable = 0x%08x\n",
- pf->msg_enable);
- }
} else if (strncmp(cmd_buf, "pfr", 3) == 0) {
dev_info(&pf->pdev->dev, "debugfs: forcing PFR\n");
i40e_do_reset_safe(pf, BIT(__I40E_PF_RESET_REQUESTED));
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index e79a920..b133a77 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -978,6 +978,10 @@ static u32 i40e_get_msglevel(struct net_device *netdev)
{
struct i40e_netdev_priv *np = netdev_priv(netdev);
struct i40e_pf *pf = np->vsi->back;
+ u32 debug_mask = pf->hw.debug_mask;
+
+ if (debug_mask)
+ netdev_info(netdev, "i40e debug_mask: 0x%08X\n", debug_mask);
return pf->msg_enable;
}
@@ -989,7 +993,8 @@ static void i40e_set_msglevel(struct net_device *netdev, u32 data)
if (I40E_DEBUG_USER & data)
pf->hw.debug_mask = data;
- pf->msg_enable = data;
+ else
+ pf->msg_enable = data;
}
static int i40e_get_regs_len(struct net_device *netdev)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b93fa2a..c5417aa 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -93,8 +93,8 @@ MODULE_DEVICE_TABLE(pci, i40e_pci_tbl);
#define I40E_MAX_VF_COUNT 128
static int debug = -1;
-module_param(debug, int, 0);
-MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+module_param(debug, uint, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all), Debug mask (0x8XXXXXXX)");
MODULE_AUTHOR("Intel Corporation, <e1000-devel@lists.sourceforge.net>");
MODULE_DESCRIPTION("Intel(R) Ethernet Connection XL710 Network Driver");
@@ -8481,15 +8481,6 @@ static int i40e_sw_init(struct i40e_pf *pf)
int err = 0;
int size;
- pf->msg_enable = netif_msg_init(I40E_DEFAULT_MSG_ENABLE,
- (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK));
- if (debug != -1 && debug != I40E_DEFAULT_MSG_ENABLE) {
- if (I40E_DEBUG_USER & debug)
- pf->hw.debug_mask = debug;
- pf->msg_enable = netif_msg_init((debug & ~I40E_DEBUG_USER),
- I40E_DEFAULT_MSG_ENABLE);
- }
-
/* Set default capability flags */
pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
I40E_FLAG_MSI_ENABLED |
@@ -10790,10 +10781,12 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
mutex_init(&hw->aq.asq_mutex);
mutex_init(&hw->aq.arq_mutex);
- if (debug != -1) {
- pf->msg_enable = pf->hw.debug_mask;
- pf->msg_enable = debug;
- }
+ pf->msg_enable = netif_msg_init(debug,
+ NETIF_MSG_DRV |
+ NETIF_MSG_PROBE |
+ NETIF_MSG_LINK);
+ if (debug < -1)
+ pf->hw.debug_mask = debug;
/* do a special CORER for clearing PXE mode once at init */
if (hw->revision_id == 0 &&
^ permalink raw reply related
* Re: [PATCH 3/3] netfilter: xt_hashlimit: uses div_u64 for division
From: Eric Dumazet @ 2016-09-30 16:38 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, Joshua Hunt, Vishwanath Pai, netfilter-devel,
coreteam, netdev, linux-kernel
In-Reply-To: <20160930160559.4102745-3-arnd@arndb.de>
On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote:
> The newly added support for high-resolution pps rates introduced multiple 64-bit
> division operations in one function, which fails on all 32-bit architectures:
>
> net/netfilter/xt_hashlimit.o: In function `user2credits':
> xt_hashlimit.c:(.text.user2credits+0x3c): undefined reference to `__aeabi_uldivmod'
> xt_hashlimit.c:(.text.user2credits+0x68): undefined reference to `__aeabi_uldivmod'
> xt_hashlimit.c:(.text.user2credits+0x88): undefined reference to `__aeabi_uldivmod'
>
> This replaces the division with an explicit call to div_u64 for version 2
> to documents that this is a slow operation, and reverts back to 32-bit arguments
> for the version 1 data to restore the original faster 32-bit division.
>
> With both changes combined, we no longer get a link error.
>
> Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to support higher pps rates")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Vishwanath Pai already sent a patch for this, and I did my version independently.
> The difference is that his version also the more expensive division for the
> version 1 variant that doesn't need it.
>
> See also http://patchwork.ozlabs.org/patch/676713/
> ---
> net/netfilter/xt_hashlimit.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 44a095ecc7b7..3d5525df6eb3 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len)
> static u64 user2credits(u64 user, int revision)
> {
> if (revision == 1) {
> + u32 user32 = user; /* use 32-bit division */
> +
This looks dangerous to me. Have you really tried all possible cases ?
Caller (even if using revision == 1) does
user2credits(cfg->avg * cfg->burst, revision);
Since this is not a fast path, I would prefer to keep the 64bit divide.
Vishwanath version looks safer.
^ permalink raw reply
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
From: Arnd Bergmann @ 2016-09-30 16:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, LKML, Moshe Shemesh,
Tariq Toukan
In-Reply-To: <CANn89iK2FQufz4NCan1y0dQME2RUOzc3NXXhqYd418ctHNc6fA@mail.gmail.com>
On Friday 30 September 2016, Eric Dumazet wrote:
> > @@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
> >
> > len++;
> > }
> > + if (len == 0)
> > + return -EINVAL;
> > +
> > err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> > ivvl[0]->qos, ivvl[0]->vlan_proto);
> > if (err < 0)
> > --
> > 2.9.0
> >
>
> So, if I read this code, we build an array, but call ndo_set_vf_vlan()
> only using first element ?
>
> Looks like the bug should be fixed in a different way.
I was wondering about this too, but didn't understand enough about it to say
if it was intentional or not. I just realized that I forgot to add Moshe and Tariq
on Cc (I relied on scripts/get_maintainer.pl, but didn't double-check).
I've added them to Cc now, hope they can clarify this.
Arnd
^ permalink raw reply
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
From: Eric Dumazet @ 2016-09-30 16:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, LKML
In-Reply-To: <20160930161416.18660-1-arnd@arndb.de>
On Fri, Sep 30, 2016 at 9:13 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> With the newly added support for IFLA_VF_VLAN_LIST netlink messages,
> we get a warning about potential uninitialized variable use in
> the parsing of the user input when enabling the -Wmaybe-uninitialized
> warning:
>
> net/core/rtnetlink.c: In function 'do_setvfinfo':
> net/core/rtnetlink.c:1756:9: error: 'ivvl$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> I have not been able to prove whether it is possible to arrive in
> this code with an empty IFLA_VF_VLAN_LIST block, but if we do,
> then ndo_set_vf_vlan gets called with uninitialized arguments.
>
> This adds an explicit check for an empty list, making it obvious
> to the reader and the compiler that this cannot happen.
>
> Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> net/core/rtnetlink.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 3ac8946bf244..b06d2f46b83e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>
> len++;
> }
> + if (len == 0)
> + return -EINVAL;
> +
> err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> ivvl[0]->qos, ivvl[0]->vlan_proto);
> if (err < 0)
> --
> 2.9.0
>
So, if I read this code, we build an array, but call ndo_set_vf_vlan()
only using first element ?
Looks like the bug should be fixed in a different way.
^ permalink raw reply
* [PATCH 2/2] mlxsw: spectrum_router: avoid potential uninitialized data usage
From: Arnd Bergmann @ 2016-09-30 16:17 UTC (permalink / raw)
To: Jiri Pirko, Ido Schimmel
Cc: Arnd Bergmann, David S. Miller, Yotam Gigi, Nogah Frankel, netdev,
linux-kernel
In-Reply-To: <20160930161734.70928-1-arnd@arndb.de>
If fi->fib_nhs is zero, the router interface pointer is uninitialized, as shown by
this warning:
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c: In function 'mlxsw_sp_router_fib_event':
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c:1674:21: error: 'r' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c:1643:23: note: 'r' was declared here
This changes the loop so we handle the case the same way as finding no router
interface pointer attached to one of the nexthops to ensure we always
trap here instead of using uninitialized data.
Fixes: b45f64d16d45 ("mlxsw: spectrum_router: Use FIB notifications instead of switchdev calls")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 48d50efec5e2..78fc557d6dd7 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -1640,7 +1640,7 @@ mlxsw_sp_router_fib4_entry_init(struct mlxsw_sp *mlxsw_sp,
struct mlxsw_sp_fib_entry *fib_entry)
{
struct fib_info *fi = fen_info->fi;
- struct mlxsw_sp_rif *r;
+ struct mlxsw_sp_rif *r = NULL;
int nhsel;
int err;
@@ -1664,11 +1664,15 @@ mlxsw_sp_router_fib4_entry_init(struct mlxsw_sp *mlxsw_sp,
* to us. Set trap and pass the packets for
* this prefix to kernel.
*/
- fib_entry->type = MLXSW_SP_FIB_ENTRY_TYPE_TRAP;
- return 0;
+ break;
}
}
+ if (!r) {
+ fib_entry->type = MLXSW_SP_FIB_ENTRY_TYPE_TRAP;
+ return 0;
+ }
+
if (fi->fib_scope != RT_SCOPE_UNIVERSE) {
fib_entry->type = MLXSW_SP_FIB_ENTRY_TYPE_LOCAL;
fib_entry->rif = r->rif;
--
2.9.0
^ permalink raw reply related
* [PATCH 1/2] net/mlx5e: shut up maybe-uninitialized warning
From: Arnd Bergmann @ 2016-09-30 16:17 UTC (permalink / raw)
To: Saeed Mahameed, Matan Barak, Leon Romanovsky
Cc: Arnd Bergmann, David S. Miller, Or Gerlitz, Amir Vadai,
Maor Gottlieb, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Build-testing this driver with -Wmaybe-uninitialized gives a new false-positive
warning that I can't really explain:
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'mlx5e_configure_flower':
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:509:3: error: 'old_attr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
It's obvious from the code that 'old_attr' is initialized whenever 'old'
is non-NULL here. The warning appears with all versions I tested from gcc-4.7
through gcc-6.1, and I could not come up with a way to rewrite the function
in a more readable way that avoids the warning, so I'm adding another
initialization to shut it up.
Fixes: 8b32580df1cb ("net/mlx5e: Add TC vlan action for SRIOV offloads")
Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index a350b7171e3d..ce8c54d18906 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -451,7 +451,7 @@ int mlx5e_configure_flower(struct mlx5e_priv *priv, __be16 protocol,
struct mlx5e_tc_flow *flow;
struct mlx5_flow_spec *spec;
struct mlx5_flow_rule *old = NULL;
- struct mlx5_esw_flow_attr *old_attr;
+ struct mlx5_esw_flow_attr *old_attr = NULL;
struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
if (esw && esw->mode == SRIOV_OFFLOADS)
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox