* [PATCH net-next 2/7] vxlan: simplify exception handling
From: Pravin B Shelar @ 2016-11-04 20:51 UTC (permalink / raw)
To: netdev; +Cc: Pravin B Shelar
In-Reply-To: <1478292716-54649-1-git-send-email-pshelar@ovn.org>
vxlan egress path error handling has became complicated, it
need to handle IPv4 and IPv6 tunnel cases.
Earlier patch removes vlan handling from vxlan_build_skb(), so
vxlan_build_skb does not need to free skb and we can simplify
the xmit path by having single error handling for both type of
tunnels.
Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
drivers/net/vxlan.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 756d826..a1e707f 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1789,7 +1789,7 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
return 0;
out_free:
- kfree_skb(skb);
+ dst_release(dst);
return err;
}
@@ -1927,7 +1927,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
struct ip_tunnel_info *info;
struct vxlan_dev *vxlan = netdev_priv(dev);
struct sock *sk;
- struct rtable *rt = NULL;
const struct iphdr *old_iph;
union vxlan_addr *dst;
union vxlan_addr remote_ip, local_ip;
@@ -2009,6 +2008,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
if (dst->sa.sa_family == AF_INET) {
struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
+ struct rtable *rt;
if (!sock4)
goto drop;
@@ -2030,7 +2030,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
netdev_dbg(dev, "circular route to %pI4\n",
&dst->sin.sin_addr.s_addr);
dev->stats.collisions++;
- goto rt_tx_error;
+ ip_rt_put(rt);
+ goto tx_error;
}
/* Bypass encapsulation if the destination is local */
@@ -2058,7 +2059,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
err = vxlan_build_skb(skb, &rt->dst, sizeof(struct iphdr),
vni, md, flags, udp_sum);
if (err < 0)
- goto xmit_tx_error;
+ goto tx_error;
udp_tunnel_xmit_skb(rt, sk, skb, src->sin.sin_addr.s_addr,
dst->sin.sin_addr.s_addr, tos, ttl, df,
@@ -2117,11 +2118,9 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
skb_scrub_packet(skb, xnet);
err = vxlan_build_skb(skb, ndst, sizeof(struct ipv6hdr),
vni, md, flags, udp_sum);
- if (err < 0) {
- dst_release(ndst);
- dev->stats.tx_errors++;
- return;
- }
+ if (err < 0)
+ goto tx_error;
+
udp_tunnel6_xmit_skb(ndst, sk, skb, dev,
&src->sin6.sin6_addr,
&dst->sin6.sin6_addr, tos, ttl,
@@ -2133,17 +2132,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
drop:
dev->stats.tx_dropped++;
- goto tx_free;
+ dev_kfree_skb(skb);
+ return;
-xmit_tx_error:
- /* skb is already freed. */
- skb = NULL;
-rt_tx_error:
- ip_rt_put(rt);
tx_error:
dev->stats.tx_errors++;
-tx_free:
- dev_kfree_skb(skb);
+ kfree_skb(skb);
}
/* Transmit local packets over Vxlan
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 0/7] vxlan: General improvements.
From: Pravin B Shelar @ 2016-11-04 20:51 UTC (permalink / raw)
To: netdev; +Cc: Pravin B Shelar
Following patch series improves vxlan fast path, removes
duplicate code and simplifies vxlan xmit code path.
Pravin B Shelar (7):
vxlan: avoid vlan processing in vxlan device.
vxlan: simplify exception handling
vxlan: avoid checking socket multiple times.
vxlan: improve vxlan route lookup checks.
vxlan: simplify RTF_LOCAL handling.
vxlan: simplify vxlan xmit
vxlan: remove unsed vxlan_dev_dst_port()
drivers/net/vxlan.c | 264 ++++++++++++++++++++++--------------------------
include/linux/if_vlan.h | 16 ---
include/net/vxlan.h | 10 --
3 files changed, 123 insertions(+), 167 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH net-next 1/7] vxlan: avoid vlan processing in vxlan device.
From: Pravin B Shelar @ 2016-11-04 20:51 UTC (permalink / raw)
To: netdev; +Cc: Pravin B Shelar
In-Reply-To: <1478292716-54649-1-git-send-email-pshelar@ovn.org>
VxLan device does not have special handling for vlan taging on egress.
Therefore it does not make sense to expose vlan offloading feature.
Signed-off-by: Pravin B Shelar <pshelar@ovn.org>
---
drivers/net/vxlan.c | 9 +--------
include/linux/if_vlan.h | 16 ----------------
2 files changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index cb5cc7c..756d826 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1748,18 +1748,13 @@ static int vxlan_build_skb(struct sk_buff *skb, struct dst_entry *dst,
}
min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len
- + VXLAN_HLEN + iphdr_len
- + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
+ + VXLAN_HLEN + iphdr_len;
/* Need space for new headers (invalidates iph ptr) */
err = skb_cow_head(skb, min_headroom);
if (unlikely(err))
goto out_free;
- skb = vlan_hwaccel_push_inside(skb);
- if (WARN_ON(!skb))
- return -ENOMEM;
-
err = iptunnel_handle_offloads(skb, type);
if (err)
goto out_free;
@@ -2527,10 +2522,8 @@ static void vxlan_setup(struct net_device *dev)
dev->features |= NETIF_F_GSO_SOFTWARE;
dev->vlan_features = dev->features;
- dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
dev->hw_features |= NETIF_F_GSO_SOFTWARE;
- dev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
netif_keep_dst(dev);
dev->priv_flags |= IFF_NO_QUEUE;
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 3319d97..8d5fcd6 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -399,22 +399,6 @@ static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
skb->vlan_tci = 0;
return skb;
}
-/*
- * vlan_hwaccel_push_inside - pushes vlan tag to the payload
- * @skb: skbuff to tag
- *
- * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
- * VLAN tag from @skb->vlan_tci inside to the payload.
- *
- * Following the skb_unshare() example, in case of error, the calling function
- * doesn't have to worry about freeing the original skb.
- */
-static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
-{
- if (skb_vlan_tag_present(skb))
- skb = __vlan_hwaccel_push_inside(skb);
- return skb;
-}
/**
* __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net] r8152: Fix broken RX checksums.
From: Mark Lord @ 2016-11-04 20:13 UTC (permalink / raw)
To: Hayes Wang, David Miller
Cc: nic_swsd, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <d1ac848d-d36d-c81a-e0a2-4cab2d7382cd@pobox.com>
On 16-11-04 09:50 AM, Mark Lord wrote:
> Yeah, the device or driver is definitely getting confused with rx_desc structures.
> I added code to check for unlikely rx_desc values, and it found this for starters:
>
> rx_desc: 00480801 00480401 00480001 0048fc00 0048f800 0048f400 pkt_len=2045
> rx_data: 00 f0 48 00 00 ec 48 00 00 e8 48 00 00 e4 48 00 00 e0 48 00 00 dc 48 00 00 d8 48 00 00 d4
> 48 00
> rx_data: 00 d0 48 00 00 cc 48 00 00 c8 48 00 00 c4 48 00 00 c0 48 00 00 bc 48 00 00 b8 48 00 00 b4
> 48 00
> rx_data: 00 b0 48 00 00 ac 48 00 00 01 00 00 81 ed 00 00 00 01 00 00 00 00 00 00 00 00 00 02 4d ac
> 00 00
> rx_data: 10 00 ff ff ff ff 00 00 01 28 83 d6 ff 6d 00 20 25 b1 58 1b 68 ff 00 05 20 01 56 41 17 35
> 00 00
> ...
>
> The MTU/MRU on this link is the standard 1500 bytes, so a pkt_len of 2045 isn't valid here.
> And the rx_desc values look an awful lot like the rx_data values that follow it.
>
> There's definitely more broken here than just TCP RX checksums.
I spent a bit more time on this again today, and made progress.
The issue seems to be stale rx buffers.
I'll discuss further offline with Hayes Wang.
--
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com
^ permalink raw reply
* Re: Coding Style: Reverse XMAS tree declarations ?
From: Lino Sanfilippo @ 2016-11-04 20:06 UTC (permalink / raw)
To: Joe Perches, David Miller, lsanfil
Cc: madalin.bucur, akpm, corbet, netdev, linuxppc-dev, linux-kernel,
oss, ppc, pebolle, joakim.tjernlund, Randy Dunlap
In-Reply-To: <1478281455.1924.41.camel@perches.com>
On 04.11.2016 18:44, Joe Perches wrote:
> On Fri, 2016-11-04 at 11:07 -0400, David Miller wrote:
>> From: Lino Sanfilippo <lsanfil@marvell.com>
>> > On 04.11.2016 07:53, Joe Perches wrote:
>> >> CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to
>> >> shortest
>> >> #446: FILE: drivers/net/ethernet/ethoc.c:446:
>> >> + int size = bd.stat >> 16;
>> >> + struct sk_buff *skb;
>> > should not this case be valid? Optically the longer line is already
>> > before the shorter.
>> > I think that the whole point in using this reverse xmas tree ordering
>> > is to have
>> > the code optically tidied up and not to enforce ordering between
>> > variable name lengths.
>>
>> That's correct.
>
> And also another reason the whole reverse xmas tree
> automatic declaration layout concept is IMO dubious.
>
> Basically, you're looking not at the initial ordering
> of automatics as important, but helping find a specific
> automatic when reversing from reading code is not always
> correct.
>
> Something like:
>
> static void function{args,...)
> {
> [longish list of reverse xmas tree identifiers...]
> struct foo *bar = longish_function(args, ...);
> struct foobarbaz *qux;
> [more identifers]
>
> [multiple screenfuls of code later...)
>
> new_function(..., bar, ...);
>
> [more code...]
> }
>
> and the reverse xmas tree helpfulness of looking up the
> type of bar is neither obvious nor easy.
>
In this case it is IMHO rather the declaration + initialization that makes
"bar" hard to find at one glance, not the use of RXT. You could do something like
[longish list of reverse xmas tree identifiers...]
struct foobarbaz *qux;
struct foo *bar;
bar = longish_function(args, ...);
to increase readability.
Personally I find it more readable to always use a separate line for initializations
by means of functions (regardless of whether the RXT scheme is used or not).
> My preference would be for a bar that serves coffee and alcohol.
>
At least a bar like this should not be too hard to find :)
Regards,
Lino
^ permalink raw reply
* Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)
From: David VomLehn @ 2016-11-04 19:48 UTC (permalink / raw)
To: Randy Dunlap
Cc: Joe Perches, David Miller, madalin.bucur, Andrew Morton,
Jonathan Corbet, netdev, linuxppc-dev, linux-kernel, oss, ppc,
pebolle, joakim.tjernlund
In-Reply-To: <5d4925c3-e6c1-8780-37bf-6d529f128cd9@infradead.org>
On Fri, Nov 04, 2016 at 10:05:15AM -0700, Randy Dunlap wrote:
> On 11/03/16 23:53, Joe Perches wrote:
> > On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> >> From: Madalin Bucur <madalin.bucur@nxp.com>
> >> Date: Wed, 2 Nov 2016 22:17:26 +0200
> >>
> >>> This introduces the Freescale Data Path Acceleration Architecture
> >>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> >>> +{
> >>> + u8 i;
> >>> + size_t res = DPAA_BP_RAW_SIZE / 2;
> >>
> >> Always order local variable declarations from longest to shortest line,
> >> also know as Reverse Christmas Tree Format.
> >
> > I think this declaration sorting order is misguided but
> > here's a possible change to checkpatch adding a test for it
> > that does this test just for net/ and drivers/net/
>
> I agree with the misguided part.
> That's not actually in CodingStyle AFAICT. Where did this come from?
>
>
> thanks.
> --
> ~Randy
This puzzles me. The CodingStyle gives some pretty reasonable rationales
for coding style over above the "it's easier to read if it all looks the
same". I can see rationales for other approaches (and I am not proposing
any of these):
alphabetic order Easier to search for declarations
complex to simple As in, structs and unions, pointers to simple
data (int, char), simple data. It seems like I
can deduce the simple types from usage, but more
complex I need to know things like the
particular structure.
group by usage Mirror the ontological locality in the code
Do we have a basis for thinking this is easier or more consistent than
any other approach?
--
David VL
^ permalink raw reply
* [PATCH net] sock: fix sendmmsg for partial sendmsg
From: Soheil Hassas Yeganeh @ 2016-11-04 19:36 UTC (permalink / raw)
To: davem, netdev; +Cc: edumazet, willemb, ncardwell, Soheil Hassas Yeganeh
From: Soheil Hassas Yeganeh <soheil@google.com>
Do not send the next message in sendmmsg for partial sendmsg
invocations.
sendmmsg assumes that it can continue sending the next message
when the return value of the individual sendmsg invocations
is positive. It results in corrupting the data for TCP,
SCTP, and UNIX streams.
For example, sendmmsg([["abcd"], ["efgh"]]) can result in a stream
of "aefgh" if the first sendmsg invocation sends only the first
byte while the second sendmsg goes through.
Datagram sockets either send the entire datagram or fail, so
this patch affects only sockets of type SOCK_STREAM and
SOCK_SEQPACKET.
Fixes: 228e548e6020 ("net: Add sendmmsg socket system call")
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
net/socket.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/socket.c b/net/socket.c
index 5a9bf5e..272518b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2038,6 +2038,8 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
if (err)
break;
++datagrams;
+ if (msg_data_left(&msg_sys))
+ break;
cond_resched();
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH net-next] sock: do not set sk_err in sock_dequeue_err_skb
From: Willem de Bruijn @ 2016-11-04 19:26 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Soheil Hassas Yeganeh, David Miller, Network Development,
Eric Dumazet, Willem de Bruijn, Neal Cardwell,
Soheil Hassas Yeganeh, Andy Lutomirski
In-Reply-To: <a3f7ea70-658e-9836-1a09-d1b9586582eb@stressinduktion.org>
On Thu, Nov 3, 2016 at 7:10 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> [also cc'ed Andy, albeit this doesn't seem to solve his initial problem,
> right? <http://www.spinics.net/lists/netdev/msg331753.html>]
Indeed, this does not help disambiguate the source of an error
returned by a socketcall. It only reduces how often sk_err is leaked
into the socket call datapath. Local errors and timestamps never set
sk_err in the first place, so there can be no expectation that they
set it on dequeue.
I suspect that that line in sock_dequeue_err_skb originates from icmp
errors, as those do set sk_err whenever queueing an error. So
reenabling it when there are multiple errors on the queue makes sense,
if all queued errors would be icmp errors. But they are not. And that
path is racy, so there is no guarantee, either way. Processes that
rely on syscall blocking to read the errqueue still block for icmp
errors after this patch, due to the initial sk_err assignment on
enqueue.
Note that the comment in the patch that only TCP socket calls are
blocked is probably incorrect. TCP has its own check in tcp_sendmsg
and returns EPIPE when an sk_err is set, without clearing it. But
other send, recv, connect, .. implementations include a path to
sock_error that aborts the call (e.g., in __skb_try_recv_datagram),
returns the value of sk_err, and clears it. This is an inconsistency
between the protocols.
Andy's original problem mentions recvmsg. We could probably make that
unambiguous by returning a cmsg with the value of sk_err if RECVERR is
set and sk_err is the source of the error. But this still does not
solve the general issue, as other socketcalls, like send and connect,
can also return sk_err. We can modify the number of places that check
for it and currently abort system calls. Mainly sock_error(). One
option is to add a socket option (SOL_SOCKET, as sk_err is not limited
to when INET_RECVERR is set) to make that a noop. If the caller sets
this option it instead has to wait with poll to receive POLLERR and
call getsockopt SO_ERROR and recv MSG_ERRQUEUE to get the asynchronous
error.
^ permalink raw reply
* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Alexander Duyck @ 2016-11-04 19:13 UTC (permalink / raw)
To: Jason Baron; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <0e1e6651-3de6-6a71-cb37-be2942fb70b2@akamai.com>
On Fri, Nov 4, 2016 at 12:07 PM, Jason Baron <jbaron@akamai.com> wrote:
>
>
> On 11/04/2016 02:43 PM, Alexander Duyck wrote:
>>
>> On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
>>>
>>> From: Jason Baron <jbaron@akamai.com>
>>>
>>> When read() is called on /proc/net/route requesting a size that is one
>>> entry size (128 bytes) less than m->size or greater, the resulting output
>>> has missing and/or duplicate entries. Since m->size is typically
>>> PAGE_SIZE,
>>> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
>>> bytes will see bogus output.
>>>
>>> For example:
>>>
>>> for i in {100..200}; do
>>> ip route add 192.168.1.$i dev eth0
>>> done
>>> dd if=/proc/net/route of=/tmp/good bs=1024
>>> dd if=/proc/net/route of=/tmp/bad bs=4096
>>>
>>> # diff -q /tmp/good /tmp/bad
>>> Files /tmp/good and /tmp/bad differ
>>>
>>> I think this has gone unnoticed, since the output of 'netstat -r' and
>>> 'route' is generated by reading in 1,024 byte increments and thus not
>>> corrupted. Further, the number of entries in the route table needs to be
>>> sufficiently large in order to trigger the problematic case.
>>>
>>> The issue arises because fib_route_get_idx() does not properly handle
>>> the case where pos equals iter->pos. This case only arises when we have
>>> a large read buffer size because we end up re-requesting the last entry
>>> that overflowed m->buf. In the case of a smaller read buffer size,
>>> we don't exceed the size of m->buf, and thus fib_route_get_idx() is
>>> called
>>> with pos greater than iter->pos.
>>>
>>> Fix by properly handling the iter->pos == pos case.
>>>
>>> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in
>>> /proc/net/route")
>>> Cc: Andy Whitcroft <apw@canonical.com>
>>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>> ---
>>> net/ipv4/fib_trie.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>>> index 31cef3602585..1017533fc75c 100644
>>> --- a/net/ipv4/fib_trie.c
>>> +++ b/net/ipv4/fib_trie.c
>>> @@ -2411,12 +2411,17 @@ static struct key_vector
>>> *fib_route_get_idx(struct fib_route_iter *iter,
>>> loff_t pos)
>>> {
>>> struct key_vector *l, **tp = &iter->tnode;
>>> + loff_t saved_pos = 0;
>>> t_key key;
>>>
>>> /* use cache location of next-to-find key */
>>> if (iter->pos > 0 && pos >= iter->pos) {
>>> pos -= iter->pos;
>>> key = iter->key;
>>> + if (pos == 0) {
>>> + saved_pos = iter->pos;
>>> + key--;
>>> + }
>>> } else {
>>> iter->pos = 0;
>>> key = 0;
>>> @@ -2436,10 +2441,13 @@ static struct key_vector
>>> *fib_route_get_idx(struct fib_route_iter *iter,
>>> break;
>>> }
>>>
>>> - if (l)
>>> + if (l) {
>>> iter->key = key; /* remember it */
>>> - else
>>> + if (saved_pos)
>>> + iter->pos = saved_pos;
>>> + } else {
>>> iter->pos = 0; /* forget it */
>>> + }
>>>
>>> return l;
>>> }
>>
>>
>> This doesn't seem correct to me. I will have to look through this.
>> My understanding is that the value of iter->pos is supposed to be the
>> next position for us to grab, not the last one that was retrieved. If
>> we are trying to re-request the last value then we should be falling
>> back into the else case for this since pos should be one less than
>> iter->pos. The problem is the table could change out from under us
>> which is one of the reasons why we don't want to try and rewind the
>> key like you are doing here.
>>
>> - Alex
>>
>
> Hi Alex,
>
> In this case, seq_read() has called m->op->next(), which sets iter->pos
> equal to pos and iter->key to key + 1. However, when we then go to output
> the item associated with key, the 'm->op->next()' call overflows. Thus, we
> have a situation where iter->pos equals pos, iter->key = key + 1, but we
> have not displayed the item at position 'key' (thus the bug is that we miss
> the item at key).
>
> The change I proposed was simply to restart the search from 'key' in this
> case. If that item has disappeared, we will output the next one, or if its
> been replaced we will display its replacement. I think that is
> ok?
>
> The bug could also be fixed by changing:
>
> if (iter->pos > 0 && pos >= iter->pos) {
>
> to say:
>
> if (iter->pos > 0 && pos > iter->pos) {
>
> But that restarts the search on every overflow, which could mean every page
> size, and that seems suboptimal to me. Like-wise, if we make pos 1 less than
> iter->pos that restarts the search. The idea with this patch is to not force
> us to redo the entire search on each overflow.
>
> Thanks,
>
> -Jason
Actually I think the underlying issue is that we still have an
unresolved off by one error. Specifically offset 0 actually
represents two values, the header for the file and entry 0. I think
the fix is for us to look at pushing pos to 1 for the start of the
data, and we reserve POS 0 for SEQ_START_TOKEN. Doing that we should
start at the correct offset for each section following the first page.
- Alex
^ permalink raw reply
* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Jason Baron @ 2016-11-04 19:07 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <CAKgT0Ud7BVHfzbT=CsXn--_WSSt=hofcM3uNJcxdY=kaJ5psOA@mail.gmail.com>
On 11/04/2016 02:43 PM, Alexander Duyck wrote:
> On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
>> From: Jason Baron <jbaron@akamai.com>
>>
>> When read() is called on /proc/net/route requesting a size that is one
>> entry size (128 bytes) less than m->size or greater, the resulting output
>> has missing and/or duplicate entries. Since m->size is typically PAGE_SIZE,
>> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
>> bytes will see bogus output.
>>
>> For example:
>>
>> for i in {100..200}; do
>> ip route add 192.168.1.$i dev eth0
>> done
>> dd if=/proc/net/route of=/tmp/good bs=1024
>> dd if=/proc/net/route of=/tmp/bad bs=4096
>>
>> # diff -q /tmp/good /tmp/bad
>> Files /tmp/good and /tmp/bad differ
>>
>> I think this has gone unnoticed, since the output of 'netstat -r' and
>> 'route' is generated by reading in 1,024 byte increments and thus not
>> corrupted. Further, the number of entries in the route table needs to be
>> sufficiently large in order to trigger the problematic case.
>>
>> The issue arises because fib_route_get_idx() does not properly handle
>> the case where pos equals iter->pos. This case only arises when we have
>> a large read buffer size because we end up re-requesting the last entry
>> that overflowed m->buf. In the case of a smaller read buffer size,
>> we don't exceed the size of m->buf, and thus fib_route_get_idx() is called
>> with pos greater than iter->pos.
>>
>> Fix by properly handling the iter->pos == pos case.
>>
>> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in /proc/net/route")
>> Cc: Andy Whitcroft <apw@canonical.com>
>> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
>> net/ipv4/fib_trie.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 31cef3602585..1017533fc75c 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -2411,12 +2411,17 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>> loff_t pos)
>> {
>> struct key_vector *l, **tp = &iter->tnode;
>> + loff_t saved_pos = 0;
>> t_key key;
>>
>> /* use cache location of next-to-find key */
>> if (iter->pos > 0 && pos >= iter->pos) {
>> pos -= iter->pos;
>> key = iter->key;
>> + if (pos == 0) {
>> + saved_pos = iter->pos;
>> + key--;
>> + }
>> } else {
>> iter->pos = 0;
>> key = 0;
>> @@ -2436,10 +2441,13 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
>> break;
>> }
>>
>> - if (l)
>> + if (l) {
>> iter->key = key; /* remember it */
>> - else
>> + if (saved_pos)
>> + iter->pos = saved_pos;
>> + } else {
>> iter->pos = 0; /* forget it */
>> + }
>>
>> return l;
>> }
>
> This doesn't seem correct to me. I will have to look through this.
> My understanding is that the value of iter->pos is supposed to be the
> next position for us to grab, not the last one that was retrieved. If
> we are trying to re-request the last value then we should be falling
> back into the else case for this since pos should be one less than
> iter->pos. The problem is the table could change out from under us
> which is one of the reasons why we don't want to try and rewind the
> key like you are doing here.
>
> - Alex
>
Hi Alex,
In this case, seq_read() has called m->op->next(), which sets iter->pos
equal to pos and iter->key to key + 1. However, when we then go to
output the item associated with key, the 'm->op->next()' call overflows.
Thus, we have a situation where iter->pos equals pos, iter->key = key +
1, but we have not displayed the item at position 'key' (thus the bug is
that we miss the item at key).
The change I proposed was simply to restart the search from 'key' in
this case. If that item has disappeared, we will output the next one, or
if its been replaced we will display its replacement. I think that is
ok?
The bug could also be fixed by changing:
if (iter->pos > 0 && pos >= iter->pos) {
to say:
if (iter->pos > 0 && pos > iter->pos) {
But that restarts the search on every overflow, which could mean every
page size, and that seems suboptimal to me. Like-wise, if we make pos 1
less than iter->pos that restarts the search. The idea with this patch
is to not force us to redo the entire search on each overflow.
Thanks,
-Jason
^ permalink raw reply
* Re: [PATCH net 0/6] Mellanox 100G mlx5 fixes 2016-11-04
From: David Miller @ 2016-11-04 19:00 UTC (permalink / raw)
To: saeedm; +Cc: netdev, ogerlitz, brouer
In-Reply-To: <1478216927-31328-1-git-send-email-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Fri, 4 Nov 2016 01:48:41 +0200
> This series contains six hot fixes of the mlx5 core and mlx5e driver.
>
> Huy fixed an invalid pointer dereference on initialization flow for when
> the selected mlx5 load profile is out of range.
>
> Or provided three eswitch offloads related fixes
> - Prevent changing NS of a VF representor.
> - Handle matching on vlan priority for offloaded TC rules
> - Set the actions for offloaded rules properly
>
> On my part I here addressed the error flow related issues in
> mlx5e_open_channel reported by Jesper just this week.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next resend 00/13] ring reconfiguration and XDP support
From: David Miller @ 2016-11-04 18:56 UTC (permalink / raw)
To: jakub.kicinski; +Cc: netdev
In-Reply-To: <1478193129-23476-1-git-send-email-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 3 Nov 2016 17:11:56 +0000
> This set adds support for ethtool channel API and XDP.
Series applied, thank you!
^ permalink raw reply
* [PATCH net-next 2/2] tcp: no longer hold ehash lock while calling tcp_get_info()
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
Eric Dumazet, Eric Dumazet
In-Reply-To: <1478285672-3195-1-git-send-email-edumazet@google.com>
We had various problems in the past in tcp_get_info() and used
specific synchronization to avoid deadlocks.
We would like to add more instrumentation points for TCP, and
avoiding grabing socket lock in tcp_getinfo() was too costly.
Being able to lock the socket allows to provide consistent set
of fields.
inet_diag_dump_icsk() can make sure ehash locks are not
held any more when tcp_get_info() is called.
We can remove syncp added in commit d654976cbf85
("tcp: fix a potential deadlock in tcp_get_info()"), but we need
to use lock_sock_fast() instead of spin_lock_bh() since TCP input
path can now be run from process context.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
---
include/linux/tcp.h | 2 --
net/ipv4/inet_diag.c | 48 +++++++++++++++++++++++++++++++++---------------
net/ipv4/tcp.c | 20 +++++++++-----------
net/ipv4/tcp_input.c | 4 ----
4 files changed, 42 insertions(+), 32 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a17ae7b85218..32a7c7e35b71 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -176,8 +176,6 @@ struct tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
- struct u64_stats_sync syncp; /* protects 64bit vars (cf tcp_get_info()) */
-
u32 snd_una; /* First byte we want an ack for */
u32 snd_sml; /* Last byte of the most recently transmitted small packet */
u32 rcv_tstamp; /* timestamp of last received ACK (for keepalives) */
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3b34024202d8..4dea33e5f295 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -861,10 +861,11 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
struct netlink_callback *cb,
const struct inet_diag_req_v2 *r, struct nlattr *bc)
{
+ bool net_admin = netlink_net_capable(cb->skb, CAP_NET_ADMIN);
struct net *net = sock_net(skb->sk);
- int i, num, s_i, s_num;
u32 idiag_states = r->idiag_states;
- bool net_admin = netlink_net_capable(cb->skb, CAP_NET_ADMIN);
+ int i, num, s_i, s_num;
+ struct sock *sk;
if (idiag_states & TCPF_SYN_RECV)
idiag_states |= TCPF_NEW_SYN_RECV;
@@ -877,7 +878,6 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
for (i = s_i; i < INET_LHTABLE_SIZE; i++) {
struct inet_listen_hashbucket *ilb;
- struct sock *sk;
num = 0;
ilb = &hashinfo->listening_hash[i];
@@ -922,13 +922,14 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
if (!(idiag_states & ~TCPF_LISTEN))
goto out;
+#define SKARR_SZ 16
for (i = s_i; i <= hashinfo->ehash_mask; i++) {
struct inet_ehash_bucket *head = &hashinfo->ehash[i];
spinlock_t *lock = inet_ehash_lockp(hashinfo, i);
struct hlist_nulls_node *node;
- struct sock *sk;
-
- num = 0;
+ struct sock *sk_arr[SKARR_SZ];
+ int num_arr[SKARR_SZ];
+ int idx, accum, res;
if (hlist_nulls_empty(&head->chain))
continue;
@@ -936,9 +937,12 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
if (i > s_i)
s_num = 0;
+next_chunk:
+ num = 0;
+ accum = 0;
spin_lock_bh(lock);
sk_nulls_for_each(sk, node, &head->chain) {
- int state, res;
+ int state;
if (!net_eq(sock_net(sk), net))
continue;
@@ -962,21 +966,35 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
if (!inet_diag_bc_sk(bc, sk))
goto next_normal;
- res = sk_diag_fill(sk, skb, r,
+ sock_hold(sk);
+ num_arr[accum] = num;
+ sk_arr[accum] = sk;
+ if (++accum == SKARR_SZ)
+ break;
+next_normal:
+ ++num;
+ }
+ spin_unlock_bh(lock);
+ res = 0;
+ for (idx = 0; idx < accum; idx++) {
+ if (res >= 0) {
+ res = sk_diag_fill(sk_arr[idx], skb, r,
sk_user_ns(NETLINK_CB(cb->skb).sk),
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
cb->nlh, net_admin);
- if (res < 0) {
- spin_unlock_bh(lock);
- goto done;
+ if (res < 0)
+ num = num_arr[idx];
}
-next_normal:
- ++num;
+ sock_gen_put(sk_arr[idx]);
}
-
- spin_unlock_bh(lock);
+ if (res < 0)
+ break;
cond_resched();
+ if (accum == SKARR_SZ) {
+ s_num = num + 1;
+ goto next_chunk;
+ }
}
done:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 117982be0cab..a7d54cbcdabb 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -405,7 +405,6 @@ void tcp_init_sock(struct sock *sk)
tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
tp->snd_cwnd_clamp = ~0;
tp->mss_cache = TCP_MSS_DEFAULT;
- u64_stats_init(&tp->syncp);
tp->reordering = sock_net(sk)->ipv4.sysctl_tcp_reordering;
tcp_enable_early_retrans(tp);
@@ -2710,9 +2709,8 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
const struct tcp_sock *tp = tcp_sk(sk); /* iff sk_type == SOCK_STREAM */
const struct inet_connection_sock *icsk = inet_csk(sk);
u32 now = tcp_time_stamp, intv;
- unsigned int start;
- int notsent_bytes;
u64 rate64;
+ bool slow;
u32 rate;
memset(info, 0, sizeof(*info));
@@ -2792,17 +2790,17 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
info->tcpi_total_retrans = tp->total_retrans;
- do {
- start = u64_stats_fetch_begin_irq(&tp->syncp);
- put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
- put_unaligned(tp->bytes_received, &info->tcpi_bytes_received);
- } while (u64_stats_fetch_retry_irq(&tp->syncp, start));
+ slow = lock_sock_fast(sk);
+
+ put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
+ put_unaligned(tp->bytes_received, &info->tcpi_bytes_received);
+ info->tcpi_notsent_bytes = max_t(int, 0, tp->write_seq - tp->snd_nxt);
+
+ unlock_sock_fast(sk, slow);
+
info->tcpi_segs_out = tp->segs_out;
info->tcpi_segs_in = tp->segs_in;
- notsent_bytes = READ_ONCE(tp->write_seq) - READ_ONCE(tp->snd_nxt);
- info->tcpi_notsent_bytes = max(0, notsent_bytes);
-
info->tcpi_min_rtt = tcp_min_rtt(tp);
info->tcpi_data_segs_in = tp->data_segs_in;
info->tcpi_data_segs_out = tp->data_segs_out;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index f2c59c8e57ff..a70046fea0e8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3351,9 +3351,7 @@ static void tcp_snd_una_update(struct tcp_sock *tp, u32 ack)
u32 delta = ack - tp->snd_una;
sock_owned_by_me((struct sock *)tp);
- u64_stats_update_begin_raw(&tp->syncp);
tp->bytes_acked += delta;
- u64_stats_update_end_raw(&tp->syncp);
tp->snd_una = ack;
}
@@ -3363,9 +3361,7 @@ static void tcp_rcv_nxt_update(struct tcp_sock *tp, u32 seq)
u32 delta = seq - tp->rcv_nxt;
sock_owned_by_me((struct sock *)tp);
- u64_stats_update_begin_raw(&tp->syncp);
tp->bytes_received += delta;
- u64_stats_update_end_raw(&tp->syncp);
tp->rcv_nxt = seq;
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH net-next 1/2] tcp: shortcut listeners in tcp_get_info()
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
Eric Dumazet, Eric Dumazet
In-Reply-To: <1478285672-3195-1-git-send-email-edumazet@google.com>
Being lockless in tcp_get_info() is hard, because we need to add
specific synchronization in TCP fast path, like seqcount.
Following patch will change inet_diag_dump_icsk() to no longer
hold any lock for non listeners, so that we can properly acquire
socket lock in get_tcp_info() and let it return more consistent counters.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
---
net/ipv4/tcp.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3251fe71f39f..117982be0cab 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2721,6 +2721,27 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
info->tcpi_state = sk_state_load(sk);
+ /* Report meaningful fields for all TCP states, including listeners */
+ rate = READ_ONCE(sk->sk_pacing_rate);
+ rate64 = rate != ~0U ? rate : ~0ULL;
+ put_unaligned(rate64, &info->tcpi_pacing_rate);
+
+ rate = READ_ONCE(sk->sk_max_pacing_rate);
+ rate64 = rate != ~0U ? rate : ~0ULL;
+ put_unaligned(rate64, &info->tcpi_max_pacing_rate);
+
+ info->tcpi_reordering = tp->reordering;
+ info->tcpi_snd_cwnd = tp->snd_cwnd;
+
+ if (info->tcpi_state == TCP_LISTEN) {
+ /* listeners aliased fields :
+ * tcpi_unacked -> Number of children ready for accept()
+ * tcpi_sacked -> max backlog
+ */
+ info->tcpi_unacked = sk->sk_ack_backlog;
+ info->tcpi_sacked = sk->sk_max_ack_backlog;
+ return;
+ }
info->tcpi_ca_state = icsk->icsk_ca_state;
info->tcpi_retransmits = icsk->icsk_retransmits;
info->tcpi_probes = icsk->icsk_probes_out;
@@ -2748,13 +2769,9 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
info->tcpi_snd_mss = tp->mss_cache;
info->tcpi_rcv_mss = icsk->icsk_ack.rcv_mss;
- if (info->tcpi_state == TCP_LISTEN) {
- info->tcpi_unacked = sk->sk_ack_backlog;
- info->tcpi_sacked = sk->sk_max_ack_backlog;
- } else {
- info->tcpi_unacked = tp->packets_out;
- info->tcpi_sacked = tp->sacked_out;
- }
+ info->tcpi_unacked = tp->packets_out;
+ info->tcpi_sacked = tp->sacked_out;
+
info->tcpi_lost = tp->lost_out;
info->tcpi_retrans = tp->retrans_out;
info->tcpi_fackets = tp->fackets_out;
@@ -2768,23 +2785,13 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
info->tcpi_rtt = tp->srtt_us >> 3;
info->tcpi_rttvar = tp->mdev_us >> 2;
info->tcpi_snd_ssthresh = tp->snd_ssthresh;
- info->tcpi_snd_cwnd = tp->snd_cwnd;
info->tcpi_advmss = tp->advmss;
- info->tcpi_reordering = tp->reordering;
info->tcpi_rcv_rtt = jiffies_to_usecs(tp->rcv_rtt_est.rtt)>>3;
info->tcpi_rcv_space = tp->rcvq_space.space;
info->tcpi_total_retrans = tp->total_retrans;
- rate = READ_ONCE(sk->sk_pacing_rate);
- rate64 = rate != ~0U ? rate : ~0ULL;
- put_unaligned(rate64, &info->tcpi_pacing_rate);
-
- rate = READ_ONCE(sk->sk_max_pacing_rate);
- rate64 = rate != ~0U ? rate : ~0ULL;
- put_unaligned(rate64, &info->tcpi_max_pacing_rate);
-
do {
start = u64_stats_fetch_begin_irq(&tp->syncp);
put_unaligned(tp->bytes_acked, &info->tcpi_bytes_acked);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH net-next 0/2] tcp: tcp_get_info() locking changes
From: Eric Dumazet @ 2016-11-04 18:54 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Neal Cardwell, Yuchung Cheng, Soheil Hassas Yeganeh,
Eric Dumazet, Eric Dumazet
This short series prepares tcp_get_info() for more detailed infos.
In order to not slow down fast path, our goal is to use the normal
socket spinlock instead of custom synchronization.
All we need to ensure is that tcp_get_info() is not called with
ehash lock, which might dead lock, since packet processing would acquire
the spinlocks in reverse way.
Eric Dumazet (2):
tcp: shortcut listeners in get_tcp_info()
tcp: no longer hold ehash lock while calling tcp_get_info()
include/linux/tcp.h | 2 --
net/ipv4/inet_diag.c | 48 +++++++++++++++++++++++++++++--------------
net/ipv4/tcp.c | 57 ++++++++++++++++++++++++++++------------------------
net/ipv4/tcp_input.c | 4 ----
4 files changed, 64 insertions(+), 47 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCH net-next v1 00/10] amd-xgbe: AMD XGBE driver updates 2016-11-03
From: David Miller @ 2016-11-04 18:52 UTC (permalink / raw)
To: thomas.lendacky; +Cc: netdev
In-Reply-To: <20161103181727.7383.34446.stgit@tlendack-t1.amdoffice.net>
From: Tom Lendacky <thomas.lendacky@amd.com>
Date: Thu, 3 Nov 2016 13:17:28 -0500
> This patch series is targeted at preparing the driver for a new PCI version
> of the hardware. After this series is applied, a follow-on series will
> introduce the support for the PCI version of the hardware.
>
> The following updates and fixes are included in this driver update series:
>
> - Fix formatting of PCS debug register dump
> - Prepare for priority-based FIFO allocation
> - Implement priority-based FIFO allocation
> - Prepare for working with more than one type of PCS/PHY
> - Prepare for the introduction of clause 37 auto-negotiation
> - Add support for clause 37 auto-negotiation
> - Prepare for supporting a new PCS register access method
> - Add support for 64-bit management counter registers
> - Update DMA channel status determination
> - Prepare for supporting PCI devices in addition to platform devices
>
> This patch series is based on net-next.
Series applied, thanks Tom.
^ permalink raw reply
* Re: [PATCH net-next v2] net: inet: Support UID-based routing
From: David Miller @ 2016-11-04 18:45 UTC (permalink / raw)
To: lorenzo; +Cc: netdev, ek, eric.dumazet, zenczykowski
In-Reply-To: <1478193823-60516-1-git-send-email-lorenzo@google.com>
From: Lorenzo Colitti <lorenzo@google.com>
Date: Fri, 4 Nov 2016 02:23:40 +0900
> This patchset adds support for per-UID routing.
Looks great, thanks for all of the hard work.
Series applied.
Please submit the necessary iproute2 patches, as needed.
Thanks again.
^ permalink raw reply
* Re: [PATCH net] fib_trie: correct /proc/net/route for large read buffer
From: Alexander Duyck @ 2016-11-04 18:43 UTC (permalink / raw)
To: Jason Baron; +Cc: David Miller, Netdev, Andy Whitcroft, Alexander Duyck
In-Reply-To: <1478270730-11765-1-git-send-email-jbaron@akamai.com>
On Fri, Nov 4, 2016 at 7:45 AM, Jason Baron <jbaron@akamai.com> wrote:
> From: Jason Baron <jbaron@akamai.com>
>
> When read() is called on /proc/net/route requesting a size that is one
> entry size (128 bytes) less than m->size or greater, the resulting output
> has missing and/or duplicate entries. Since m->size is typically PAGE_SIZE,
> for a PAGE_SIZE of 4,096 this means that reads requesting more than 3,968
> bytes will see bogus output.
>
> For example:
>
> for i in {100..200}; do
> ip route add 192.168.1.$i dev eth0
> done
> dd if=/proc/net/route of=/tmp/good bs=1024
> dd if=/proc/net/route of=/tmp/bad bs=4096
>
> # diff -q /tmp/good /tmp/bad
> Files /tmp/good and /tmp/bad differ
>
> I think this has gone unnoticed, since the output of 'netstat -r' and
> 'route' is generated by reading in 1,024 byte increments and thus not
> corrupted. Further, the number of entries in the route table needs to be
> sufficiently large in order to trigger the problematic case.
>
> The issue arises because fib_route_get_idx() does not properly handle
> the case where pos equals iter->pos. This case only arises when we have
> a large read buffer size because we end up re-requesting the last entry
> that overflowed m->buf. In the case of a smaller read buffer size,
> we don't exceed the size of m->buf, and thus fib_route_get_idx() is called
> with pos greater than iter->pos.
>
> Fix by properly handling the iter->pos == pos case.
>
> Fixes: 25b97c016b26 ("ipv4: off-by-one in continuation handling in /proc/net/route")
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---
> net/ipv4/fib_trie.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 31cef3602585..1017533fc75c 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2411,12 +2411,17 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
> loff_t pos)
> {
> struct key_vector *l, **tp = &iter->tnode;
> + loff_t saved_pos = 0;
> t_key key;
>
> /* use cache location of next-to-find key */
> if (iter->pos > 0 && pos >= iter->pos) {
> pos -= iter->pos;
> key = iter->key;
> + if (pos == 0) {
> + saved_pos = iter->pos;
> + key--;
> + }
> } else {
> iter->pos = 0;
> key = 0;
> @@ -2436,10 +2441,13 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter,
> break;
> }
>
> - if (l)
> + if (l) {
> iter->key = key; /* remember it */
> - else
> + if (saved_pos)
> + iter->pos = saved_pos;
> + } else {
> iter->pos = 0; /* forget it */
> + }
>
> return l;
> }
This doesn't seem correct to me. I will have to look through this.
My understanding is that the value of iter->pos is supposed to be the
next position for us to grab, not the last one that was retrieved. If
we are trying to re-request the last value then we should be falling
back into the else case for this since pos should be one less than
iter->pos. The problem is the table could change out from under us
which is one of the reasons why we don't want to try and rewind the
key like you are doing here.
- Alex
^ permalink raw reply
* Re: [PATCH net-next v2 00/11] net: dsa: mv88e6xxx: refine port operations
From: David Miller @ 2016-11-04 18:40 UTC (permalink / raw)
To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20161104022336.14273-1-vivien.didelot@savoirfairelinux.com>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Fri, 4 Nov 2016 03:23:25 +0100
> The Marvell chips have one internal SMI device per port, containing a
> set of registers used to configure a port's link, STP state, default
> VLAN or addresses database, etc.
>
> This patchset creates port files to implement the port operations as
> described in datasheets, and extend the chip ops structure with them.
>
> Patches 1 to 6 implement accessors for port's STP state, port based VLAN
> map, default FID, default VID, and 802.1Q mode.
>
> Patches 7 to 11 implement the port's MAC setup of link state, duplex
> mode, RGMII delay and speed, all accessed through port's register 0x01.
>
> The new port's MAC setup code is used to re-implement the adjust_link
> code and correctly force the link down before changing any of the MAC
> settings, as requested by the datasheets.
>
> The port's MAC accessors use values compatible with struct phy_device
> (e.g. DUPLEX_FULL) and extend them when needed (e.g. SPEED_MAX).
>
> Changes in v2:
>
> - Strictly use new _UNFORCED values instead of re-using _UNKNOWN ones.
Series applied, thanks Vivien.
^ permalink raw reply
* Re: [PATCH v2 2/2 ] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:24 UTC (permalink / raw)
To: David Miller; +Cc: f.fainelli, mans, netdev, linux-kernel, slash.tmp, andrew
In-Reply-To: <20161104.135448.1013810503648782291.davem@davemloft.net>
Hi David,
On 11/04/2016 06:54 PM, David Miller wrote:
> From: Sebastian Frias <sf84@laposte.net>
> Date: Fri, 4 Nov 2016 18:02:15 +0100
>
>> Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
>> and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
>> delay for tx and rx only") added several RGMII definitions:
>> PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
>> PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
>>
>> Those are all RGMII modes (1Gbit) and must be considered that way when
>> setting the MAC mode or the pad mode for the HW to work properly.
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>
> You cannot just repost one part of a patch series when you make changes.
>
> You must always repost the entire series as a new fresh version, with
> changelog entries added to your "[PATCH v2 0/2] ..." header posting.
>
Thanks for the information.
I sent v2, and then v3, because v2 had formatting issues, hopefully it is
ok now.
Best regards,
Sebastian
^ permalink raw reply
* [PATCH v3 2/2] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:23 UTC (permalink / raw)
To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD1AF.6030300@laposte.net>
Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
delay for tx and rx only") added several RGMII definitions:
PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
Those are all RGMII modes (1Gbit) and must be considered that way when
setting the MAC mode or the pad mode for the HW to work properly.
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index d2855c9..fba2699 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
static void nb8800_mac_config(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
+ struct phy_device *phydev = dev->phydev;
bool gigabit = priv->speed == SPEED_1000;
u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
u32 mac_mode = 0;
@@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;
if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(phydev))
mac_mode |= RGMII_MODE;
mac_mode |= GMAC_MODE;
@@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
case PHY_INTERFACE_MODE_RGMII:
- pad_mode = PAD_MODE_RGMII;
- break;
-
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
pad_mode = PAD_MODE_RGMII;
break;
--
1.7.11.2
^ permalink raw reply related
* [PATCH v3 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:22 UTC (permalink / raw)
To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD1AF.6030300@laposte.net>
The delay can be applied at PHY or MAC level, but since
PHY drivers will apply the delay at PHY level when using
one of the "internal delay" declinations of RGMII mode
(like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
at MAC level causes issues.
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index b59aa35..d2855c9 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
case PHY_INTERFACE_MODE_RGMII_TXID:
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
default:
--
1.7.11.2
^ permalink raw reply related
* [PATCH v3 0/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:21 UTC (permalink / raw)
To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
This is v3 of the series, it fixes formatting issues of v2.
In v2 of the series, only the second patch:
"net: ethernet: nb8800: handle all RGMII definitions" is modified
to account for Florian's suggestion.
Sebastian Frias (2):
net: ethernet: nb8800: Do not apply TX delay at MAC level
net: ethernet: nb8800: handle all RGMII definitions
drivers/net/ethernet/aurora/nb8800.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--
1.7.11.2
^ permalink raw reply
* [PATCH v2 2/2] net: ethernet: nb8800: handle all RGMII definitions
From: Sebastian Frias @ 2016-11-04 18:19 UTC (permalink / raw)
To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD0CA.9070506@laposte.net>
Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
delay for tx and rx only") added several RGMII definitions:
PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
Those are all RGMII modes (1Gbit) and must be considered that way when
setting the MAC mode or the pad mode for the HW to work properly.
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index d2855c9..fba2699 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
static void nb8800_mac_config(struct net_device *dev)
{
struct nb8800_priv *priv = netdev_priv(dev);
+ struct phy_device *phydev = dev->phydev;
bool gigabit = priv->speed == SPEED_1000;
u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
u32 mac_mode = 0;
@@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
mac_mode |= HALF_DUPLEX;
if (gigabit) {
- if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+ if (phy_interface_is_rgmii(phydev))
mac_mode |= RGMII_MODE;
mac_mode |= GMAC_MODE;
@@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
case PHY_INTERFACE_MODE_RGMII:
- pad_mode = PAD_MODE_RGMII;
- break;
-
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
pad_mode = PAD_MODE_RGMII;
break;
--
1.7.11.2
^ permalink raw reply related
* [PATCH v2 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
From: Sebastian Frias @ 2016-11-04 18:18 UTC (permalink / raw)
To: Måns Rullgård, David S. Miller, netdev; +Cc: LKML, Mason, Andrew Lunn
In-Reply-To: <581CD0CA.9070506@laposte.net>
The delay can be applied at PHY or MAC level, but since
PHY drivers will apply the delay at PHY level when using
one of the "internal delay" declinations of RGMII mode
(like PHY_INTERFACE_MODE_RGMII_TXID), applying it again
at MAC level causes issues.
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
drivers/net/ethernet/aurora/nb8800.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index b59aa35..d2855c9 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev)
break;
case PHY_INTERFACE_MODE_RGMII_TXID:
- pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+ pad_mode = PAD_MODE_RGMII;
break;
default:
--
1.7.11.2
^ 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