* Re: [patch] samples/bpf: silence shift wrapping warning
From: Dan Carpenter @ 2017-01-24 8:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Arnaldo Carvalho de Melo, Thomas Graf, Alexei Starovoitov,
Joe Stringer, David S. Miller, linux-kernel@vger.kernel.org,
kernel-janitors, netdev@vger.kernel.org
In-Reply-To: <CAADnVQLF6PWEjic6aSRBnRqtO25eEw7YLE+Gb95+f5_pvq3ZMA@mail.gmail.com>
On Mon, Jan 23, 2017 at 10:44:34PM -0800, Alexei Starovoitov wrote:
> On Mon, Jan 23, 2017 at 5:27 AM, Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> > Em Sun, Jan 22, 2017 at 02:51:25PM -0800, Alexei Starovoitov escreveu:
> >> On Sat, Jan 21, 2017 at 07:51:43AM +0300, Dan Carpenter wrote:
> >> > max_key is a value in the 0-63 range, so on 32 bit systems the shift
> >> > could wrap.
> >> >
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>
> >> Looks fine. I think 'net-next' is ok.
> >
> > I could process these patches, if that would help,
>
> Thanks for the offer.
> I don't think there will be conflicts with all the work happening in net-next,
> but it's best to avoid even possibility of it when we can.
> Dan,
> can you please resend the patch cc-ing Dave and netdev ?
> please mention [PATCH net-next] in the subject.
>
Sure.
regards,
dan carpenter
^ permalink raw reply
* [patch net-next] sctp: fix some debug output
From: Dan Carpenter @ 2017-01-24 9:05 UTC (permalink / raw)
To: Vlad Yasevich, Xin Long
Cc: Neil Horman, David S. Miller, linux-sctp, netdev, kernel-janitors
We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
it causes an off-by-one read overflow.
Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/sctp/debug.c b/net/sctp/debug.c
index 95d7b15dad21..c5f4ed5242ac 100644
--- a/net/sctp/debug.c
+++ b/net/sctp/debug.c
@@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
"TIMEOUT_T4_RTO",
"TIMEOUT_T5_SHUTDOWN_GUARD",
"TIMEOUT_HEARTBEAT",
+ "TIMEOUT_RECONF",
"TIMEOUT_SACK",
"TIMEOUT_AUTOCLOSE",
};
^ permalink raw reply related
* Re: [patch net-next] sctp: fix some debug output
From: Marcelo Ricardo Leitner @ 2017-01-24 9:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors, Colin King
In-Reply-To: <20170124090540.GA27652@mwanda>
On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
> We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
> it causes an off-by-one read overflow.
>
> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Weird, seems your patch is missing the --- marker here.
Colin (Cc'ed) is supposed to post a v3 of his patch containing this fix.
>
> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
> index 95d7b15dad21..c5f4ed5242ac 100644
> --- a/net/sctp/debug.c
> +++ b/net/sctp/debug.c
> @@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
> "TIMEOUT_T4_RTO",
> "TIMEOUT_T5_SHUTDOWN_GUARD",
> "TIMEOUT_HEARTBEAT",
> + "TIMEOUT_RECONF",
> "TIMEOUT_SACK",
> "TIMEOUT_AUTOCLOSE",
> };
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: wrong smp_mb__after_atomic() in tcp_check_space() ?
From: Oleg Nesterov @ 2017-01-24 9:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jason Baron, David S. Miller, Herbert Xu, Yauheni Kaliuta, netdev
In-Reply-To: <1485194671.16328.195.camel@edumazet-glaptop3.roam.corp.google.com>
On 01/23, Eric Dumazet wrote:
>
> On Mon, 2017-01-23 at 11:56 -0500, Jason Baron wrote:
> > On 01/23/2017 09:30 AM, Oleg Nesterov wrote:
> > > Hello,
> > >
> > > smp_mb__after_atomic() looks wrong and misleading, sock_reset_flag() does the
> > > non-atomic __clear_bit() and thus it can not guarantee test_bit(SOCK_NOSPACE)
> > > (non-atomic too) won't be reordered.
> > >
> >
> > Indeed. Here's a bit of discussion on it:
> > http://marc.info/?l=linux-netdev&m=146662325920596&w=2
> >
> > > It was added by 3c7151275c0c9a "tcp: add memory barriers to write space paths"
> > > and the patch looks correct in that we need the barriers in tcp_check_space()
> > > and tcp_poll() in theory, so it seems tcp_check_space() needs smp_mb() ?
> > >
> >
> > Yes, I think it should be upgraded to an smp_mb() there. If you agree
> > with this analysis, I will send a patch to upgrade it. Note, I did not
> > actually run into this race in practice.
>
> SOCK_QUEUE_SHRUNK is used locally in TCP, it is not used by tcp_poll().
>
> (Otherwise it would be using atomic set/clear operations)
>
> I do not see obvious reason why we have this smp_mb__after_atomic() in
> tcp_check_space().
It is not that we need to serialize __clear_bit(SOCK_QUEUE_SHRUNK) and
test_bit(SOCK_NOSPACE), we do not care if they are reordered.
But we need to ensure that either tcp_poll() sees sk_stream_is_writeable()
or tcp_check_space() sees SOCK_NOSPACE and calls tcp_new_space().
> But looking at this code, it seems we lack one barrier if sk_sndbuf is
> ever increased. Fortunately this almost never happen during TCP session
> lifetime...
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index bfa165cc455ad0a9aea44964aa663dbe6085aebd..3692e9f4c852cebf8c4d46c141f112e75e4ae66d 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -331,8 +331,13 @@ static void tcp_sndbuf_expand(struct sock *sk)
> sndmem = ca_ops->sndbuf_expand ? ca_ops->sndbuf_expand(sk) : 2;
> sndmem *= nr_segs * per_mss;
>
> - if (sk->sk_sndbuf < sndmem)
> + if (sk->sk_sndbuf < sndmem) {
> sk->sk_sndbuf = min(sndmem, sysctl_tcp_wmem[2]);
> + /* Paired with second sk_stream_is_writeable(sk)
> + * test from tcp_poll()
> + */
> + smp_wmb();
> + }
> }
I do not think we need the additional barrier here. If we are going to call
sk->sk_write_space() we rely on wq_has_sleeper() which has a barrier which
also pairs with the 2nd check in tcp_poll().
Oleg.
^ permalink raw reply
* Re: [PATCH net] net/mlx5e: Do not recycle pages from emergency reserve
From: Saeed Mahameed @ 2017-01-24 9:21 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Eric Dumazet, Tom Herbert, Saeed Mahameed, netdev, Tariq Toukan,
Davem
In-Reply-To: <20170123101455.380ef896@redhat.com>
On Mon, Jan 23, 2017 at 11:14 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Sat, 21 Jan 2017 11:26:49 -0800
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> > My previous measurements show approx 20℅ speedup on a UDP test with
>> > delivery to remote CPU.
>> >
>> I find this a bit strange. When you have time (ie not while driving your
>> car or during week end) please give more details, for example on message
>> size.
>
> I tested this with both 64 bytes and 1500 bytes. After I moved to 50G
> and 100G testing then I don't need to use 64 bytes packets to provoke
> the bottlenecks in the stack ;-)
>
Exactly! for XDP like uses cases, page cache maybe a non required optimization.
but when you start testing a typical TCP use cases over 50/100G link
you will need
more buffers (pages) to host the traffic for longer periods, you will
hit that bottleneck.
>> Was it before skb_condense() was added ?
>
> It tested this just before skb_condense() was added. BUT
> skb_condense() does not get activated when using mlx5, because uses
> build_skb() ie. not using frags.
>
Well, we can always replace build_skb with alloc_skb +
memcpy(skb->data, headlen) + add_skb_frag(payload)
does it it worth it ? and is it healthy that both skb->data and
skb_shinfo(skb)->frags[i] point to the same page ?
> For people that don't realize this:
> Eric's optimization in skb_condense() is about trading remote CPU
> atomic refcnt (put_page) for copy + local CPU refcnt dec.
>
> My measurements show cycles cost local=31 vs. remote=208, thus a
> estimated saving around 177 cycles. Which is spend on calling a fairly
> complex function __pskb_pull_tail(), and only works for more complex
> SKBs with frags.
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH][V3][net-next] net: sctp: fix array overrun read on sctp_timer_tbl
From: Colin King @ 2017-01-24 9:25 UTC (permalink / raw)
To: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
add this in. Also compare timeout with the size of the array
sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX. Also add
a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
so we don't ever get this kind of mismatch between the table
and SCTP_EVENT_TIMEOUT_MAX in the future.
Kudos to Marcelo Ricardo Leitner for spotting the missing string
and suggesting the build time sanity check.
Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/sctp/debug.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/sctp/debug.c b/net/sctp/debug.c
index 95d7b15..2e47eb2 100644
--- a/net/sctp/debug.c
+++ b/net/sctp/debug.c
@@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
"TIMEOUT_T4_RTO",
"TIMEOUT_T5_SHUTDOWN_GUARD",
"TIMEOUT_HEARTBEAT",
+ "TIMEOUT_RECONF",
"TIMEOUT_SACK",
"TIMEOUT_AUTOCLOSE",
};
@@ -166,7 +167,9 @@ static const char *const sctp_timer_tbl[] = {
/* Lookup timer debug name. */
const char *sctp_tname(const sctp_subtype_t id)
{
- if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
+ BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl));
+
+ if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
return sctp_timer_tbl[id.timeout];
return "unknown_timer";
}
--
2.10.2
^ permalink raw reply related
* Re: [PATCH net] net/mlx5e: Do not recycle pages from emergency reserve
From: Saeed Mahameed @ 2017-01-24 9:29 UTC (permalink / raw)
To: Tom Herbert
Cc: kernel netdev, Saeed Mahameed, netdev, Tariq Toukan, Davem,
Eric Dumazet, Jesper Dangaard Brouer
In-Reply-To: <CALx6S37EsGgf69R6N5DkLQbOgiHO1J4si4LMJ1+DMjnHW41eMQ@mail.gmail.com>
On Sun, Jan 22, 2017 at 7:50 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Sat, Jan 21, 2017 at 12:31 PM, Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
>> On Sat, Jan 21, 2017 at 9:12 PM, kernel netdev <netdev@brouer.com> wrote:
>>>
>>>
>>> Den 21. jan. 2017 7.10 PM skrev "Tom Herbert" <tom@herbertland.com>:
>>>
>>> On Thu, Jan 19, 2017 at 11:14 AM, Saeed Mahameed
>>> <saeedm@dev.mellanox.co.il> wrote:
>>>> On Thu, Jan 19, 2017 at 9:03 AM, Eric Dumazet <eric.dumazet@gmail.com>
>>>> wrote:
>>>>> From: Eric Dumazet <edumazet@google.com>
>>>>>
>>>>> A driver using dev_alloc_page() must not reuse a page allocated from
>>>>> emergency memory reserve.
>>>>>
>>>>> Otherwise all packets using this page will be immediately dropped,
>>>>> unless for very specific sockets having SOCK_MEMALLOC bit set.
>>>>>
>>>>> This issue might be hard to debug, because only a fraction of received
>>>>> packets would be dropped.
>>>>
>>>> Hi Eric,
>>>>
>>>> When you say reuse, you mean point to the same page from several SKBs ?
>>>>
>>>> Because in our page cache implementation we don't reuse pages that
>>>> already passed to the stack,
>>>> we just keep them in the page cache until the ref count drop back to
>>>> one, so we recycle them (i,e they will be re-used only when no one
>>>> else is using them).
>>>>
>>> Saeed,
>>>
>>> Speaking of the mlx page cache can we remove this or a least make it
>>> optional to use. It is another example of complex functionality being
>>> put into drivers that makes things like backports more complicated and
>>
>> Re complexity, I am not sure the mlx page cache is that complex,
>> we just wrap alloc_page/put_page with our own page cache calls.
>> Roughly the page cache implementation is 200-300 LOC tops all concentrated
>> in one place in the code.
>>
> Taken as part of the RX buffer management code the whole thing in very
> complicated and seems to be completely bereft of any comments in the
> code as to how things are supposed to work.
>
The Idea is fairly simple, maybe we could have made it cleaner if we
better decoupled the page
cache from the rx logic.
The page cache works in parallel to the rx logic, it takes its owns ref counts,
this way the rx logic will continue working as if there was no cache
(get/put_pages).
but instead of of the kernel (get/put_pages) it will call the internal
cache get/put_cache_page.
Yeah I agree, some comments could help. We will work on this when we
fix the HOL issues.
^ permalink raw reply
* [PATCH net] net: reset ct before calling ndo_start_xmit
From: Paolo Abeni @ 2017-01-24 9:40 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Hannes Frederic Sowa, Florian Westphal
Some devices (e.g. ipoib and most wifi drivers) can retain the
to-be-xmitted packets on some internal queue for a possibly
unlimited time.
Removing conntrack modules after some skbs are queued on any of
those devices may cause rmmod to hang waiting for ct refcount going
away.
Since clearing skb nfct early can also improve the performance,
we now clear skb nfct before calling ndo_start_xmit() for all
the devices not strictly requiring such information, that is,
all virtual devices.
Currently we use the NETIF_F_LLTX feature bit to identify such devices,
since all the [legacy] phys drivers setting such bit are not prone
the hangup issue. The plan is adding a specific 'this is a
virtual device' priv flag and use it instead, in a later net-next
patch.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/netdevice.h | 7 +++++++
net/core/dev.c | 3 +++
2 files changed, 10 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9bde955..6e6b2ea 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4200,6 +4200,13 @@ static inline bool netif_reduces_vlan_mtu(struct net_device *dev)
return dev->priv_flags & IFF_MACSEC;
}
+/* return true if we should preserve skb nfct before calling ndo_start_xmit() */
+static inline bool netif_needs_ct(struct net_device *dev)
+{
+ /* any kind of virtual device needs to preserve the ct entry */
+ return dev->features & NETIF_F_LLTX;
+}
+
extern struct pernet_operations __net_initdata loopback_net_ops;
/* Logging, debugging and troubleshooting/diagnostic helpers. */
diff --git a/net/core/dev.c b/net/core/dev.c
index 7f218e0..85fcae0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2886,6 +2886,9 @@ static int xmit_one(struct sk_buff *skb, struct net_device *dev,
unsigned int len;
int rc;
+ if (!netif_needs_ct(dev))
+ nf_reset(skb);
+
if (!list_empty(&ptype_all) || !list_empty(&dev->ptype_all))
dev_queue_xmit_nit(skb, dev);
--
2.9.3
^ permalink raw reply related
* [RESENT PATCH net,stable] qmi_wwan/cdc_ether: add device ID for HP lt2523 (Novatel E371) WWAN card
From: Bjørn Mork @ 2017-01-24 9:45 UTC (permalink / raw)
To: netdev; +Cc: Oliver Neukum, linux-usb, Bjørn Mork, Dan Williams
Another rebranded Novatel E371. qmi_wwan should drive this device, while
cdc_ether should ignore it. Even though the USB descriptors are plain
CDC-ETHER that USB interface is a QMI interface. Ref commit 7fdb7846c9ca
("qmi_wwan/cdc_ether: add device IDs for Dell 5804 (Novatel E371) WWAN
card")
Cc: Dan Williams <dcbw@redhat.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
Resending with correct author and to address. I didn't manage to spell
kernel correctly... I guess I should have had one more cup of coffee
this morning.
No changes to the diff.
drivers/net/usb/cdc_ether.c | 8 ++++++++
drivers/net/usb/qmi_wwan.c | 7 +++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index fe7b2886cb6b..86144f9a80ee 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -531,6 +531,7 @@ static const struct driver_info wwan_info = {
#define SAMSUNG_VENDOR_ID 0x04e8
#define LENOVO_VENDOR_ID 0x17ef
#define NVIDIA_VENDOR_ID 0x0955
+#define HP_VENDOR_ID 0x03f0
static const struct usb_device_id products[] = {
/* BLACKLIST !!
@@ -677,6 +678,13 @@ static const struct usb_device_id products[] = {
.driver_info = 0,
},
+/* HP lt2523 (Novatel E371) - handled by qmi_wwan */
+{
+ USB_DEVICE_AND_INTERFACE_INFO(HP_VENDOR_ID, 0x421d, USB_CLASS_COMM,
+ USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+ .driver_info = 0,
+},
+
/* AnyDATA ADU960S - handled by qmi_wwan */
{
USB_DEVICE_AND_INTERFACE_INFO(0x16d5, 0x650a, USB_CLASS_COMM,
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 6fe1cdb0174f..24d5272cdce5 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -654,6 +654,13 @@ static const struct usb_device_id products[] = {
USB_CDC_PROTO_NONE),
.driver_info = (unsigned long)&qmi_wwan_info,
},
+ { /* HP lt2523 (Novatel E371) */
+ USB_DEVICE_AND_INTERFACE_INFO(0x03f0, 0x421d,
+ USB_CLASS_COMM,
+ USB_CDC_SUBCLASS_ETHERNET,
+ USB_CDC_PROTO_NONE),
+ .driver_info = (unsigned long)&qmi_wwan_info,
+ },
{ /* HP lt4112 LTE/HSPA+ Gobi 4G Module (Huawei me906e) */
USB_DEVICE_AND_INTERFACE_INFO(0x03f0, 0x581d, USB_CLASS_VENDOR_SPEC, 1, 7),
.driver_info = (unsigned long)&qmi_wwan_info,
--
2.11.0
^ permalink raw reply related
* [patch net-next] tipc: uninitialized return code in tipc_setsockopt()
From: Dan Carpenter @ 2017-01-24 9:49 UTC (permalink / raw)
To: Jon Maloy
Cc: Ying Xue, David S. Miller, netdev, tipc-discussion,
kernel-janitors
We shuffled some code around and added some new case statements here and
now "res" isn't initialized on all paths.
Fixes: 01fd12bb189a ("tipc: make replicast a user selectable option")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 5bec8aac5008..103d1fd058c0 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2348,7 +2348,7 @@ static int tipc_setsockopt(struct socket *sock, int lvl, int opt,
struct sock *sk = sock->sk;
struct tipc_sock *tsk = tipc_sk(sk);
u32 value = 0;
- int res;
+ int res = 0;
if ((lvl == IPPROTO_TCP) && (sock->type == SOCK_STREAM))
return 0;
@@ -2388,7 +2388,6 @@ static int tipc_setsockopt(struct socket *sock, int lvl, int opt,
break;
case TIPC_CONN_TIMEOUT:
tipc_sk(sk)->conn_timeout = value;
- /* no need to set "res", since already 0 at this point */
break;
case TIPC_MCAST_BROADCAST:
tsk->mc_method.rcast = false;
^ permalink raw reply related
* Re: [patch net-next] sctp: fix some debug output
From: Dan Carpenter @ 2017-01-24 9:51 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors, Colin King
In-Reply-To: <20170124091411.GE3781@localhost.localdomain>
On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
> > We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
> > it causes an off-by-one read overflow.
> >
> > Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Weird, seems your patch is missing the --- marker here.
> Colin (Cc'ed) is supposed to post a v3 of his patch containing this fix.
>
Colin, if you wanted, you could CC kernel-janitors with static checker
fixes. There are several of us on that list.
regards,
dan carpenter
^ permalink raw reply
* Re: [patch net-next] sctp: fix some debug output
From: Colin Ian King @ 2017-01-24 9:52 UTC (permalink / raw)
To: Dan Carpenter, Marcelo Ricardo Leitner
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors
In-Reply-To: <20170124095104.GA4149@mwanda>
On 24/01/17 09:51, Dan Carpenter wrote:
> On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
>>> We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
>>> it causes an off-by-one read overflow.
>>>
>>> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> Weird, seems your patch is missing the --- marker here.
>> Colin (Cc'ed) is supposed to post a v3 of his patch containing this fix.
>>
>
> Colin, if you wanted, you could CC kernel-janitors with static checker
> fixes. There are several of us on that list.
OK, thanks for the heads-up on that Dan.
Colin
>
> regards,
> dan carpenter
>
^ permalink raw reply
* Re: 4.9.0-rc8: tg3 dead after resume
From: Siva Reddy Kallam @ 2017-01-24 9:55 UTC (permalink / raw)
To: Billy Shuman; +Cc: Michael Chan, Netdev
On Mon, Jan 16, 2017 at 8:29 PM, Billy Shuman <wshuman3@gmail.com> wrote:
> On Mon, Jan 16, 2017 at 2:15 AM, Siva Reddy Kallam
> <siva.kallam@broadcom.com> wrote:
>> Hi,
>>
>> Sorry for the late reply.
>> We don't have exact model.
>> We tried to reproduce with HP elitebook folio laptop & HP elite thunderbolt
>> 3 dock.
>> We observed issue with Thunder bolt bridge. All the connected devices on the
>> TH3 dock don't function at all.
>> Are you observing same issue? Did you get a chance to check with HP on this?
>
> No the only device that does not function is the nic and only after resume, it
> functions prior to that. I have a usb keyboard and three monitors
> attached to the
> dock that function properly before and after resume. Did you turn off
> authentication for thunderbolt in bios?
Do you mean Thunderbolt Security Level to “PCIe and DisplayPort - No Security”?
If Yes, We already set the same and using it.
As we are unable to reproduce, Can you please check with HP once on this?
>
>> As per our discussion with HP, Thunder bolt bridge driver is provided for
>> windows OS only.
>>
>> Regards,
>> Siva
>> ----Original Message-----
>> From: Billy Shuman [mailto:wshuman3@gmail.com]
>> Sent: Wednesday, December 28, 2016 7:33 PM
>> To: Siva Reddy Kallam <siva.kallam@broadcom.com>
>> Subject: Re: 4.9.0-rc8: tg3 dead after resume
>>
>> On Mon, Dec 26, 2016 at 1:27 AM, Siva Reddy Kallam
>> <siva.kallam@broadcom.com> wrote:
>>> On Mon, Dec 12, 2016 at 3:53 PM, Siva Reddy Kallam
>>> <siva.kallam@broadcom.com> wrote:
>>>> On Fri, Dec 9, 2016 at 7:59 PM, Billy Shuman <wshuman3@gmail.com> wrote:
>>>>> On Thu, Dec 8, 2016 at 4:03 AM, Siva Reddy Kallam
>>>>> <siva.kallam@broadcom.com> wrote:
>>>>>> On Thu, Dec 8, 2016 at 12:14 AM, Billy Shuman <wshuman3@gmail.com>
>>>>>> wrote:
>>>>>>> On Wed, Dec 7, 2016 at 12:37 PM, Michael Chan
>>>>>>> <michael.chan@broadcom.com> wrote:
>>>>>>>> On Wed, Dec 7, 2016 at 7:20 AM, Billy Shuman <wshuman3@gmail.com>
>>>>>>>> wrote:
>>>>>>>>> After resume on 4.9.0-rc8 tg3 is dead.
>>>>>>>>>
>>>>>>>>> In logs I see:
>>>>>>>>> kernel: tg3 0000:44:00.0: phy probe failed, err -19
>>>>>>>>> kernel: tg3 0000:44:00.0: Problem fetching invariants of chip,
>>>>>>>>> aborting
>>>>>>>>
>>>>>>>> -19 is -ENODEV which means tg3 cannot read the PHY ID.
>>>>>>>>
>>>>>>>> If it's a true suspend/resume operation, the driver does not have
>>>>>>>> to go through probe during resume. Please explain how you do
>>>>>>>> suspend/resume.
>>>>>>>>
>>>>>>>
>>>>>>> Sorry my previous message was accidentally sent to early.
>>>>>>>
>>>>>>> I used systemd (systemctl suspend) to suspend.
>>>>>>>
>>>>>> We need more information to proceed further.
>>>>>> Without suspend, Are you able to use the tg3 port?
>>>>>
>>>>> Yes the port works fine without suspend.
>>>> OK
>>>>>
>>>>>> Which Broadcom card are you having in laptop?
>>>>>
>>>>> The nic is a NetXtreme BCM57762 Gigabit Ethernet PCIe in a thunderbolt3
>>>>> dock.
>>>>>
>>>> OK
>>>>>> Please provide complete tg3 specific logs in dmesg.
>>>>>>
>>>>>
>>>>> [ 32.084010] tg3.c:v3.137 (May 11, 2014)
>>>>> [ 32.124695] tg3 0000:44:00.0 eth0: Tigon3 [partno(BCM957762) rev
>>>>> 57766001] (PCI Express) MAC address 98:e7:f4:8b:13:19
>>>>> [ 32.124698] tg3 0000:44:00.0 eth0: attached PHY is 57765
>>>>> (10/100/1000Base-T Ethernet) (WireSpeed[1], EEE[1])
>>>>> [ 32.124699] tg3 0000:44:00.0 eth0: RXcsums[1] LinkChgREG[0]
>>>>> MIirq[0] ASF[0] TSOcap[1]
>>>>> [ 32.124700] tg3 0000:44:00.0 eth0: dma_rwctrl[00000001]
>>>>> dma_mask[64-bit]
>>>>> [ 32.219764] tg3 0000:44:00.0 enp68s0: renamed from eth0
>>>>> [ 36.219245] tg3 0000:44:00.0 enp68s0: Link is up at 1000 Mbps, full
>>>>> duplex
>>>>> [ 36.219250] tg3 0000:44:00.0 enp68s0: Flow control is on for TX and
>>>>> on for RX
>>>>> [ 36.219251] tg3 0000:44:00.0 enp68s0: EEE is disabled
>>>>>
>>>>> after resume
>>>>> [ 92.292838] tg3 0000:44:00.0 enp68s0: No firmware running
>>>>> [ 93.521744] tg3 0000:44:00.0: tg3_abort_hw timed out,
>>>>> TX_MODE_ENABLE will not clear MAC_TX_MODE=ffffffff [ 106.704655]
>>>>> tg3 0000:44:00.0 enp68s0: Link is down [ 108.370356] tg3
>>>>> 0000:44:00.0: tg3_abort_hw timed out, TX_MODE_ENABLE will not clear
>>>>> MAC_TX_MODE=ffffffff
>>>>>
>>>>> after rmmod, modprobe
>>>>> [ 570.933636] tg3 0000:44:00.0: tg3_abort_hw timed out,
>>>>> TX_MODE_ENABLE will not clear MAC_TX_MODE=ffffffff [ 604.847215]
>>>>> tg3.c:v3.137 (May 11, 2014) [ 605.010075] tg3 0000:44:00.0: phy
>>>>> probe failed, err -19 [ 605.010077] tg3 0000:44:00.0: Problem
>>>>> fetching invariants of chip, aborting
>>>>>
>>>>>
>>>>>
>>>>>
>>>> We will try to reproduce and update you on this.
>>> We are unable to reproduce this issue with Ubuntu 16.10 (K4.8.0-22)
>>> kernel.
>>> We are in the process of verifying with 4.9.0-rc8 kernel and let you
>>> know the feedback.
>>> Can you please let us know the make/model of your laptop and procedure
>>> followed to enable tg3 driver?
>>
>> The model is an HP Zbook Studio G3.
>> The NIC is in the dock which is an HP Zbook Thunderbolt 3 dock.
>>
>> The module is loaded via udev.
>> $ cat config | grep CONFIG_TIGON3
>> CONFIG_TIGON3=m
>>
>>>>>>>> Did this work before? There has been very few changes to tg3
>>>>>>>> recently.
>>>>>>>>
>>>>>>>
>>>>>>> This is a new laptop for me, but the same behavior is seen on 4.4.36
>>>>>>> and 4.8.12.
>>>>>>>
>>>>>>>>>
>>>>>>>>> rmmod and modprobe does not fix the problem only a reboot resolves
>>>>>>>>> the issue.
>>>>>>>>>
>>>>>>>>> Billy
^ permalink raw reply
* RE: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags
From: Bharat Kumar Gogada @ 2017-01-24 10:03 UTC (permalink / raw)
To: Larry Finger, chaoming_li@realsil.com.cn,
linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: kvalo@codeaurora.org, netdev@vger.kernel.org, Ravikiran Gummaluri
In-Reply-To: <07fff87e-4dc2-f7b1-76f1-263be2410905@lwfinger.net>
> Subject: Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts
> after enabling local irq flags
>
> On 01/20/2017 08:14 AM, Bharat Kumar Gogada wrote:
> > > On 01/19/2017 04:14 AM, Bharat Kumar Gogada wrote:
> >>> -Realtek 8192CE chipset maintains local irq flags after
> >>> enabling/disabling hardware interrupts.
> >>> -Hardware interrupts are enabled before enabling the local irq
> >>> flags(these flags are being checked in interrupt handler), leading
> >>> to race condition on some RP, where the irq line between bridge and
> >>> GIC goes high at ASSERT_INTx and goes low only at DEASSERT_INTx. In
> >>> this kind of RP by the time ASSERT_INTx is seen irq_enable flag is
> >>> still set to false, resulting in continuous interrupts seen by CPU
> >>> as DEASSERT_INTx cannot be sent since flag is still false and making
> >>> CPU stall.
> >>> -Changing the sequence of setting these irq flags.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>
> >> This patch should be enhanced with the smb_xx() calls as suggested by by
> Lino.
> >>
> >> The subject should be changed. I would suggest something like "rtlwifi:
> >> rtl8192ce: Prevent race condition when enabling interrupts", as it
> >> explains the condition you are preventing.
> >>
> >> The other PCI drivers also have the same problem. Do you want to
> >> prepare the patches, or should I do it?
> >>
> > Thanks Larry. Please send out the patches adding the above enhancements
> suggested by Lino.
>
> I have prepared a patch fixing all the drivers. By the way, what CPU hardware
> showed this problem?
>
Thanks Larry, it was showed in ZynqUltrascalePlus Root Port hardware.
^ permalink raw reply
* Re: [patch net-next] sctp: fix some debug output
From: Dan Carpenter @ 2017-01-24 10:50 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors, Colin King
In-Reply-To: <20170124091411.GE3781@localhost.localdomain>
On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
> > We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
> > it causes an off-by-one read overflow.
> >
> > Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Weird, seems your patch is missing the --- marker here.
Are there tools that require the --- marker? I normally leave it out
when it's not required.
regards,
dan carpenter
^ permalink raw reply
* Re: Reference counting struct inet_peer
From: David Windsor @ 2017-01-24 11:05 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev, Kees Cook, Reshetova, Elena, Hans Liljestrand
In-Reply-To: <alpine.LFD.2.11.1701240853370.1779@ja.home.ssi.bg>
On Tue, Jan 24, 2017 at 2:38 AM, Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Mon, 23 Jan 2017, David Windsor wrote:
>
>> Hi,
>>
>> I'm working on a patchset that adds overflow protection to kernel
>> reference counters, as part of the KSPP effort. We're introducing a
>> new type, tentatively called refcount_t, that will ultimately replace
>> atomic_t as the type used for kernel reference counters. refcount_t
>> has a constrained interface relative to atomic_t and stores reference
>> counts as unsigned integers.
>>
>> While performing an audit of kernel reference counters, we've come
>> upon a few corner cases that we're unable to cleanly migrate to
>> refcount_t. One of these is the reference counting scheme for struct
>> inet_peer.
>
> ...
>
>> We're also seeing the same thing (freeing shared objects when their
>> refcount becomes -1) in ip_vs.h:
>>
>> http://lxr.free-electrons.com/source/include/net/ip_vs.h#L1424
>>
>> static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
>> {
>> if (atomic_dec_return(&dest->refcnt) < 0)
>> kfree(dest);
>> }
>
> I think, this is easy to fix. The problem is that
> dest_trash currently holds deleted dests (unlinked from RCU lists)
> with refcnt=0. If we change the dest_trash to hold dest
> with refcnt=1, the above atomic_dec_return can be changed to
> atomic_dec_and_test. Change should be small: ip_vs_dest_put
> should be removed from __ip_vs_del_dest(), ip_vs_dest_hold()
> from ip_vs_trash_get_dest() and refcnt check in
> ip_vs_dest_trash_expire() should be updated. Let me know if
> this holds your work, I can provide such patch to fix it.
>
Thanks for looking into this. Your solution does indeed solve the
problem we're looking to fix. Essentially, we just need the reference
count to never become < 0. If you have a patch to fix this, that
would be great.
Thanks,
David Windsor
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH net-next v6 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-24 11:09 UTC (permalink / raw)
To: Simon Horman
Cc: davem, netdev, jiri, paulb, john.fastabend, mrv, hadarh, ogerlitz,
roid, xiyou.wangcong, daniel
In-Reply-To: <20170123125838.GD31958@penelope.horms.nl>
On 17-01-23 07:58 AM, Simon Horman wrote:
> Hi Jamal,
>
> On Sun, Jan 22, 2017 at 03:25:50PM -0500, Jamal Hadi Salim wrote:
>
> ...
>
>> @@ -33,6 +34,8 @@ static void free_tcf(struct rcu_head *head)
>>
>> free_percpu(p->cpu_bstats);
>> free_percpu(p->cpu_qstats);
>> + kfree(p->act_cookie->data);
>
> Does the above need to be protected by a check for p->act_cookie being non-NULL?
>
Indeed it does.
>> + kfree(p->act_cookie);
>> kfree(p);
>> }
>>
>
> ...
>
>> @@ -575,6 +584,33 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>> if (err < 0)
>> goto err_mod;
>>
>> + if (tb[TCA_ACT_COOKIE]) {
>> + int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>> +
>> + if (cklen > TC_COOKIE_MAX_SIZE) {
>> + err = -EINVAL;
>> + tcf_hash_release(a, bind);
>> + goto err_mod;
>> + }
>> +
>> + a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
>> + if (!a->act_cookie) {
>> + err = -ENOMEM;
>> + tcf_hash_release(a, bind);
>> + goto err_mod;
>> + }
>> +
>> + a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE],
>> + GFP_KERNEL);
>> + if (!a->act_cookie->data) {
>> + err = -ENOMEM;
>> + kfree(a->act_cookie);
>> + tcf_hash_release(a, bind);
>> + goto err_mod;
>> + }
>> + a->act_cookie->len = cklen;
>
> FWIW, the above looks correct but it also looks like the error handling
> could be done less verbosely if the logic was moved to a separate function.
>
I will make the change.
Thanks Simon.
cheers,
jamal
^ permalink raw reply
* TCP stops sending packets over loopback on 4.10-rc3?
From: Josef Bacik @ 2017-01-24 11:20 UTC (permalink / raw)
To: Linux Netdev List; +Cc: Lawrence Brakmo, Kernel Team
Hello,
I've been trying to test some NBD changes I had made recently and I
started having packet timeouts. I traced this down to tcp just
stopping sending packets after a lot of writing. All NBD does is call
kernel_sendmsg() with a request struct and some pages when it does
writes. I did a bunch of tracing and I've narrowed it down to running
out of sk_wmem_queued space. In tcp_sendmsg() here
new_segment:
/* Allocate new segment. If the interface is SG,
* allocate skb fitting to single page.
*/
if (!sk_stream_memory_free(sk))
goto wait_for_sndbuf;
we hit this pretty regularly, and eventually just get stuck in
sk_stream_wait_memory until the timeout ends and we error out
everything. Now sk_stream_memory_free checks the sk_wmem_queued and
calls into the sk_prot->stream_memory_free(), so I broke this out like
the following
if (sk->sk_wmem_queued >= sk->sk_sndbuf) {
trace_printk("sk_wmem_queued %d, sk_sndbuf %d\n",
sk->sk_wmem_queued, sk->sk_sndbuf);
goto wait_for_sndbuf;
}
if (sk->sk_prot->stream_memory_free &&
!sk->sk_prot->stream_memory_free(sk)) {
trace_printk("sk_stream_memory_free\n");
goto wait_for_sndbuf;
}
And I got this in my tracing
kworker/u16:5-112 [001] .... 1375.637564: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [001] .... 1375.639657: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [003] .... 1375.641128: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [003] .... 1375.643441: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [001] .... 1375.807614: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [001] .... 1377.538744: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [001] .... 1377.543418: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/2:4H-1535 [002] .... 1377.544685: tcp_sendmsg:
sk_wmem_queued 4204872, sk_sndbuf 4194304
kworker/u16:5-112 [000] .... 1379.378352: tcp_sendmsg:
sk_wmem_queued 4205796, sk_sndbuf 4194304
kworker/u16:5-112 [003] .... 1380.985721: tcp_sendmsg:
sk_wmem_queued 4212416, sk_sndbuf 4194304
This is as far as I've gotten and I'll keep digging into it, but I was
wondering if this looks familiar to anybody? Also one thing I've
noticed is sk_stream_wait_memory() will wait on sk_sleep(sk), but
basically nothing wakes this up. For example it seems the main way we
reduce sk_wmem_queued is through sk_wmem_free_skb(), which doesn't
appear to wake anything up in any of its callers, so anybody who does
end up sleeping will basically never wake up. That seems like it
should be more broken than it is, so I'm curious to know how things are
actually woken up in this case. Thanks,
Josef
^ permalink raw reply
* [PATCH] cxgbit: use T6 specific macro to set force bit
From: Varun Prakash @ 2017-01-24 11:37 UTC (permalink / raw)
To: bart.vanassche; +Cc: target-devel, netdev, indranil, varun
For T6 adapters use T6 specific macro to set
force bit.
Signed-off-by: Varun Prakash <varun@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 4 ++++
drivers/target/iscsi/cxgbit/cxgbit_target.c | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
index a267173..21fc2fe 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
@@ -1349,6 +1349,10 @@ struct cpl_tx_data {
#define TX_FORCE_S 13
#define TX_FORCE_V(x) ((x) << TX_FORCE_S)
+#define T6_TX_FORCE_S 20
+#define T6_TX_FORCE_V(x) ((x) << T6_TX_FORCE_S)
+#define T6_TX_FORCE_F T6_TX_FORCE_V(1U)
+
enum {
ULP_TX_MEM_READ = 2,
ULP_TX_MEM_WRITE = 3,
diff --git a/drivers/target/iscsi/cxgbit/cxgbit_target.c b/drivers/target/iscsi/cxgbit/cxgbit_target.c
index 8bcb9b7..bcf94aa 100644
--- a/drivers/target/iscsi/cxgbit/cxgbit_target.c
+++ b/drivers/target/iscsi/cxgbit/cxgbit_target.c
@@ -162,12 +162,14 @@ cxgbit_tx_data_wr(struct cxgbit_sock *csk, struct sk_buff *skb, u32 dlen,
u32 len, u32 credits, u32 compl)
{
struct fw_ofld_tx_data_wr *req;
+ const struct cxgb4_lld_info *lldi = &csk->com.cdev->lldi;
u32 submode = cxgbit_skcb_submode(skb);
u32 wr_ulp_mode = 0;
u32 hdr_size = sizeof(*req);
u32 opcode = FW_OFLD_TX_DATA_WR;
u32 immlen = 0;
- u32 force = TX_FORCE_V(!submode);
+ u32 force = is_t5(lldi->adapter_type) ? TX_FORCE_V(!submode) :
+ T6_TX_FORCE_F;
if (cxgbit_skcb_flags(skb) & SKCBF_TX_ISO) {
opcode = FW_ISCSI_TX_DATA_WR;
--
2.0.2
^ permalink raw reply related
* Re: [PATCH net-next v6 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-24 11:46 UTC (permalink / raw)
To: Daniel Borkmann, Simon Horman
Cc: davem, netdev, jiri, paulb, john.fastabend, mrv, hadarh, ogerlitz,
roid, xiyou.wangcong
In-Reply-To: <58862CE7.2080906@iogearbox.net>
On 17-01-23 11:18 AM, Daniel Borkmann wrote:
> On 01/23/2017 01:58 PM, Simon Horman wrote:
>> Hi Jamal,
>>
>> On Sun, Jan 22, 2017 at 03:25:50PM -0500, Jamal Hadi Salim wrote:
>>
>> ...
>>
>>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>> index cd08df9..58cf1c5 100644
>>> --- a/net/sched/act_api.c
>>> +++ b/net/sched/act_api.c
>>> @@ -24,6 +24,7 @@
>>> #include <net/net_namespace.h>
>>> #include <net/sock.h>
>>> #include <net/sch_generic.h>
>>> +#include <net/pkt_cls.h>
>>> #include <net/act_api.h>
>>> #include <net/netlink.h>
>>>
>>> @@ -33,6 +34,8 @@ static void free_tcf(struct rcu_head *head)
>>>
>>> free_percpu(p->cpu_bstats);
>>> free_percpu(p->cpu_qstats);
>>> + kfree(p->act_cookie->data);
>>
>> Does the above need to be protected by a check for p->act_cookie being
>> non-NULL?
>
> Yep, that would be a NULL-deref. Why not just embedd tc_cookie as
> suggested earlier, the struct is rather small anyway ...
>
Everytime I make a change like that i seem to forget to run one
more test and it creates more bugs (example, the last two resends
are errors introduced by changing the struct from last one which
was your suggestion;->).
So you will have to forgive me I am not going back to that
definition; I will post version 7 soon.
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next v2 1/4] net: Introduce psample, a new genetlink channel for packet sampling
From: Simon Horman @ 2017-01-24 11:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, yotamg, idosch, eladr, nogahf, ogerlitz, jhs,
geert+renesas, stephen, xiyou.wangcong, linux, roopa,
john.fastabend, mrv
In-Reply-To: <20170124083734.GB1891@nanopsycho>
On Tue, Jan 24, 2017 at 09:37:34AM +0100, Jiri Pirko wrote:
> Tue, Jan 24, 2017 at 09:32:05AM CET, simon.horman@netronome.com wrote:
> >On Mon, Jan 23, 2017 at 11:07:08AM +0100, Jiri Pirko wrote:
> >> From: Yotam Gigi <yotamg@mellanox.com>
> >>
> >> Add a general way for kernel modules to sample packets, without being tied
> >> to any specific subsystem. This netlink channel can be used by tc,
> >> iptables, etc. and allow to standardize packet sampling in the kernel.
> >>
> >> For every sampled packet, the psample module adds the following metadata
> >> fields:
> >>
> >> PSAMPLE_ATTR_IIFINDEX - the packets input ifindex, if applicable
> >>
> >> PSAMPLE_ATTR_OIFINDEX - the packet output ifindex, if applicable
> >>
> >> PSAMPLE_ATTR_ORIGSIZE - the packet's original size, in case it has been
> >> truncated during sampling
> >>
> >> PSAMPLE_ATTR_SAMPLE_GROUP - the packet's sample group, which is set by the
> >> user who initiated the sampling. This field allows the user to
> >> differentiate between several samplers working simultaneously and
> >> filter packets relevant to him
> >>
> >> PSAMPLE_ATTR_GROUP_SEQ - sequence counter of last sent packet. The
> >> sequence is kept for each group
> >>
> >> PSAMPLE_ATTR_SAMPLE_RATE - the sampling rate used for sampling the packets
> >>
> >> PSAMPLE_ATTR_DATA - the actual packet bits
> >>
> >> The sampled packets are sent to the PSAMPLE_NL_MCGRP_SAMPLE multicast
> >> group. In addition, add the GET_GROUPS netlink command which allows the
> >> user to see the current sample groups, their refcount and sequence number.
> >> This command currently supports only netlink dump mode.
> >>
> >> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> >Hi Jiri, Hi Yotam,
> >
> >this looks good to me.
> >
> >Reviewed-by: Simon Horman <simon.horman@netronome.com>
> >
> >I do, however, have one question: what is your feeling about allowing
> >the per-action cooking which Jamal has proposed[1] to be emited as
> >metadata as another PSAMPLE_ATTR_* attribute? For one thing I think it
> >would allow for smooth integration with OvS user-space which makes use of a
> >cookie.
>
> Yeah, cookie could be easily added.
Great. We can address that as follow-up work at some point.
^ permalink raw reply
* Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
From: Simon Horman @ 2017-01-24 11:49 UTC (permalink / raw)
To: Yotam Gigi
Cc: Jiri Pirko, netdev@vger.kernel.org, davem@davemloft.net,
Ido Schimmel, Elad Raz, Nogah Frankel, Or Gerlitz,
jhs@mojatatu.com, geert+renesas@glider.be,
stephen@networkplumber.org, xiyou.wangcong@gmail.com,
linux@roeck-us.net, roopa@cumulusnetworks.com,
john.fastabend@gmail.com, mrv@mojatatu.com
In-Reply-To: <DB3PR05MB076447766A61C7CD63F5875CAC750@DB3PR05MB0764.eurprd05.prod.outlook.com>
On Tue, Jan 24, 2017 at 08:39:37AM +0000, Yotam Gigi wrote:
> >-----Original Message-----
> >From: Simon Horman [mailto:simon.horman@netronome.com]
> >Sent: Tuesday, January 24, 2017 10:33 AM
> >To: Jiri Pirko <jiri@resnulli.us>
> >Cc: netdev@vger.kernel.org; davem@davemloft.net; Yotam Gigi
> ><yotamg@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> ><eladr@mellanox.com>; Nogah Frankel <nogahf@mellanox.com>; Or Gerlitz
> ><ogerlitz@mellanox.com>; jhs@mojatatu.com; geert+renesas@glider.be;
> >stephen@networkplumber.org; xiyou.wangcong@gmail.com; linux@roeck-us.net;
> >roopa@cumulusnetworks.com; john.fastabend@gmail.com; mrv@mojatatu.com
> >Subject: Re: [patch net-next v2 2/4] net/sched: Introduce sample tc action
> >
> >On Mon, Jan 23, 2017 at 11:07:09AM +0100, Jiri Pirko wrote:
> >> From: Yotam Gigi <yotamg@mellanox.com>
> >>
> >> This action allows the user to sample traffic matched by tc classifier.
> >> The sampling consists of choosing packets randomly and sampling them using
> >> the psample module. The user can configure the psample group number, the
> >> sampling rate and the packet's truncation (to save kernel-user traffic).
> >>
> >> Example:
> >> To sample ingress traffic from interface eth1, one may use the commands:
> >>
> >> tc qdisc add dev eth1 handle ffff: ingress
> >>
> >> tc filter add dev eth1 parent ffff: \
> >> matchall action sample rate 12 group 4
> >>
> >> Where the first command adds an ingress qdisc and the second starts
> >> sampling randomly with an average of one sampled packet per 12 packets on
> >> dev eth1 to psample group 4.
> >>
> >> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> >
> >Reviewed-by: Simon Horman <simon.horman@netronome.com>
> >
> >Is the tc user-space (iproute2) code available yet?
>
> Yes it is. I thought about sending it the moment the kernel patches gets
> accepted.
>
> Do you want it to send it directly to you?
I'm happy to wait for now.
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 15/21] RDMA/bnxt_re: Support post_recv
From: Leon Romanovsky @ 2017-01-24 11:59 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford, linux-rdma, netdev, michael.chan, Eddie Wai,
Devesh Sharma, Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-16-git-send-email-selvin.xavier@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]
On Wed, Dec 21, 2016 at 03:42:04AM -0800, Selvin Xavier wrote:
> Enables the fastpath verb ib_post_recv.
>
> v3: Fixes sparse warnings
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 123 +++++++++++++++++++++++++++++++
> drivers/infiniband/hw/bnxt_re/ib_verbs.h | 2 +
> drivers/infiniband/hw/bnxt_re/main.c | 2 +
> drivers/infiniband/hw/bnxt_re/qplib_fp.c | 100 +++++++++++++++++++++++++
> drivers/infiniband/hw/bnxt_re/qplib_fp.h | 8 ++
> 5 files changed, 235 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index e659490..7476994c 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -1637,6 +1637,51 @@ static int bnxt_re_build_qp1_send_v2(struct bnxt_re_qp *qp,
> return rc;
> }
>
> +/* For the MAD layer, it only provides the recv SGE the size of
> + * ib_grh + MAD datagram. No Ethernet headers, Ethertype, BTH, DETH,
> + * nor RoCE iCRC. The Cu+ solution must provide buffer for the entire
> + * receive packet (334 bytes) with no VLAN and then copy the GRH
> + * and the MAD datagram out to the provided SGE.
> + */
> +static int bnxt_re_build_qp1_shadow_qp_recv(struct bnxt_re_qp *qp,
> + struct ib_recv_wr *wr,
> + struct bnxt_qplib_swqe *wqe,
> + int payload_size)
> +{
> + struct bnxt_qplib_sge ref, sge;
> + u32 rq_prod_index;
> + struct bnxt_re_sqp_entries *sqp_entry;
> +
> + rq_prod_index = bnxt_qplib_get_rq_prod_index(&qp->qplib_qp);
> +
> + if (bnxt_qplib_get_qp1_rq_buf(&qp->qplib_qp, &sge)) {
> + /* Create 1 SGE to receive the entire
> + * ethernet packet
> + */
> + /* Save the reference from ULP */
> + ref.addr = wqe->sg_list[0].addr;
> + ref.lkey = wqe->sg_list[0].lkey;
> + ref.size = wqe->sg_list[0].size;
> +
> + sqp_entry = &qp->rdev->sqp_tbl[rq_prod_index];
> +
> + /* SGE 1 */
> + wqe->sg_list[0].addr = sge.addr;
> + wqe->sg_list[0].lkey = sge.lkey;
> + wqe->sg_list[0].size = BNXT_QPLIB_MAX_QP1_RQ_HDR_SIZE_V2;
> + sge.size -= wqe->sg_list[0].size;
> +
> + sqp_entry->sge.addr = ref.addr;
> + sqp_entry->sge.lkey = ref.lkey;
> + sqp_entry->sge.size = ref.size;
> + /* Store the wrid for reporting completion */
> + sqp_entry->wrid = wqe->wr_id;
> + /* change the wqe->wrid to table index */
> + wqe->wr_id = rq_prod_index;
> + }
> + return 0;
> +}
> +
> static int is_ud_qp(struct bnxt_re_qp *qp)
> {
> return qp->qplib_qp.type == CMDQ_CREATE_QP_TYPE_UD;
> @@ -1983,6 +2028,84 @@ int bnxt_re_post_send(struct ib_qp *ib_qp, struct ib_send_wr *wr,
> return rc;
> }
>
> +static int bnxt_re_post_recv_shadow_qp(struct bnxt_re_dev *rdev,
> + struct bnxt_re_qp *qp,
> + struct ib_recv_wr *wr)
> +{
> + struct bnxt_qplib_swqe wqe;
> + int rc = 0, payload_sz = 0;
> +
> + memset(&wqe, 0, sizeof(wqe));
> + while (wr) {
> + /* House keeping */
> + memset(&wqe, 0, sizeof(wqe));
> +
> + /* Common */
> + wqe.num_sge = wr->num_sge;
> + if (wr->num_sge > qp->qplib_qp.rq.max_sge) {
> + dev_err(rdev_to_dev(rdev),
> + "Limit exceeded for Receive SGEs");
> + rc = -EINVAL;
> + goto bad;
"goto bad" directly to other place in while() to call the "break".
It will be more convenient to call "break" here.
> + }
> + payload_sz = bnxt_re_build_sgl(wr->sg_list, wqe.sg_list,
> + wr->num_sge);
> + wqe.wr_id = wr->wr_id;
> + wqe.type = BNXT_QPLIB_SWQE_TYPE_RECV;
> +
> + if (!rc)
If we are here, we will always have rc == 0.
> + rc = bnxt_qplib_post_recv(&qp->qplib_qp, &wqe);
> +bad:
> + if (rc)
> + break;
> +
> + wr = wr->next;
> + }
> + bnxt_qplib_post_recv_db(&qp->qplib_qp);
> + return rc;
> +}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH net v1 3/6] tipc: fix connection refcount error
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Until now, the generic server framework maintains the connection
id's per subscriber in server's conn_idr. At tipc_close_conn, we
remove the connection id from the server list, but the connection is
valid until we call the refcount cleanup. Hence we have a window
where the server allocates the same connection to an new subscriber
leading to inconsistent reference count. We have another refcount
warning we grab the refcount in tipc_conn_lookup() for connections
with flag with CF_CONNECTED not set. This usually occurs at shutdown
when the we stop the topology server and withdraw TIPC_CFG_SRV
publication thereby triggering a withdraw message to subscribers.
In this commit, we:
1. remove the connection from the server list at recount cleanup.
2. grab the refcount for a connection only if CF_CONNECTED is set.
Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 215849ce453d..2e803601aa99 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -91,7 +91,8 @@ static void tipc_sock_release(struct tipc_conn *con);
static void tipc_conn_kref_release(struct kref *kref)
{
struct tipc_conn *con = container_of(kref, struct tipc_conn, kref);
- struct sockaddr_tipc *saddr = con->server->saddr;
+ struct tipc_server *s = con->server;
+ struct sockaddr_tipc *saddr = s->saddr;
struct socket *sock = con->sock;
struct sock *sk;
@@ -106,6 +107,11 @@ static void tipc_conn_kref_release(struct kref *kref)
tipc_sock_release(con);
sock_release(sock);
con->sock = NULL;
+
+ spin_lock_bh(&s->idr_lock);
+ idr_remove(&s->conn_idr, con->conid);
+ s->idr_in_use--;
+ spin_unlock_bh(&s->idr_lock);
}
tipc_clean_outqueues(con);
@@ -128,8 +134,10 @@ static struct tipc_conn *tipc_conn_lookup(struct tipc_server *s, int conid)
spin_lock_bh(&s->idr_lock);
con = idr_find(&s->conn_idr, conid);
- if (con)
+ if (con && test_bit(CF_CONNECTED, &con->flags))
conn_get(con);
+ else
+ con = NULL;
spin_unlock_bh(&s->idr_lock);
return con;
}
@@ -198,15 +206,8 @@ static void tipc_sock_release(struct tipc_conn *con)
static void tipc_close_conn(struct tipc_conn *con)
{
- struct tipc_server *s = con->server;
-
if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
- spin_lock_bh(&s->idr_lock);
- idr_remove(&s->conn_idr, con->conid);
- s->idr_in_use--;
- spin_unlock_bh(&s->idr_lock);
-
/* We shouldn't flush pending works as we may be in the
* thread. In fact the races with pending rx/tx work structs
* are harmless for us here as we have already deleted this
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 4/6] tipc: fix nametbl_lock soft lockup at module exit
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Commit 333f796235a527 ("tipc: fix a race condition leading to
subscriber refcnt bug") reveals a soft lockup while acquiring
nametbl_lock.
Before commit 333f796235a527, we call tipc_conn_shutdown() from
tipc_close_conn() in the context of tipc_topsrv_stop(). In that
context, we are allowed to grab the nametbl_lock.
Commit 333f796235a527, moved tipc_conn_release (renamed from
tipc_conn_shutdown) to the connection refcount cleanup. This allows
either tipc_nametbl_withdraw() or tipc_topsrv_stop() to the cleanup.
Since tipc_exit_net() first calls tipc_topsrv_stop() and then
tipc_nametble_withdraw() increases the chances for the later to
perform the connection cleanup.
The soft lockup occurs in the call chain of tipc_nametbl_withdraw(),
when it performs the tipc_conn_kref_release() as it tries to grab
nametbl_lock again while holding it already.
tipc_nametbl_withdraw() grabs nametbl_lock
tipc_nametbl_remove_publ()
tipc_subscrp_report_overlap()
tipc_subscrp_send_event()
tipc_conn_sendmsg()
<< if (con->flags != CF_CONNECTED) we do conn_put(),
triggering the cleanup as refcount=0. >>
tipc_conn_kref_release
tipc_sock_release
tipc_conn_release
tipc_subscrb_delete
tipc_subscrp_delete
tipc_nametbl_unsubscribe << Soft Lockup >>
The previous changes in this series fixes the race conditions fixed
by commit 333f796235a527. Hence we can now revert the commit.
Fixes: 333f796235a52727 ("tipc: fix a race condition leading to subscriber refcnt bug")
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 2e803601aa99..826cde2c401e 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -86,7 +86,6 @@ struct outqueue_entry {
static void tipc_recv_work(struct work_struct *work);
static void tipc_send_work(struct work_struct *work);
static void tipc_clean_outqueues(struct tipc_conn *con);
-static void tipc_sock_release(struct tipc_conn *con);
static void tipc_conn_kref_release(struct kref *kref)
{
@@ -104,7 +103,6 @@ static void tipc_conn_kref_release(struct kref *kref)
}
saddr->scope = -TIPC_NODE_SCOPE;
kernel_bind(sock, (struct sockaddr *)saddr, sizeof(*saddr));
- tipc_sock_release(con);
sock_release(sock);
con->sock = NULL;
@@ -194,19 +192,15 @@ static void tipc_unregister_callbacks(struct tipc_conn *con)
write_unlock_bh(&sk->sk_callback_lock);
}
-static void tipc_sock_release(struct tipc_conn *con)
+static void tipc_close_conn(struct tipc_conn *con)
{
struct tipc_server *s = con->server;
- if (con->conid)
- s->tipc_conn_release(con->conid, con->usr_data);
-
- tipc_unregister_callbacks(con);
-}
-
-static void tipc_close_conn(struct tipc_conn *con)
-{
if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
+ tipc_unregister_callbacks(con);
+
+ if (con->conid)
+ s->tipc_conn_release(con->conid, con->usr_data);
/* We shouldn't flush pending works as we may be in the
* thread. In fact the races with pending rx/tx work structs
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ 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