* Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets
From: Saeed Mahameed @ 2017-08-08 16:16 UTC (permalink / raw)
To: Davide Caratti; +Cc: Tariq Toukan, Linux Netdev List, David S . Miller
In-Reply-To: <23ac3e005a8dc1a373971f1166fe6ba72d8abe4b.1501793448.git.dcaratti@redhat.com>
On Thu, Aug 3, 2017 at 11:54 PM, Davide Caratti <dcaratti@redhat.com> wrote:
> if the NIC fails to validate the checksum on TCP/UDP, and validation of IP
> checksum is successful, the driver subtracts the pseudo-header checksum
> from the value obtained by the hardware and sets CHECKSUM_COMPLETE. Don't
> do that if protocol is IPPROTO_SCTP, otherwise CRC32c validation fails.
>
> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
>
> Reported-by: Shuang Li <shuali@redhat.com>
> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM COMPLETE")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
^ permalink raw reply
* Question about via-ircc.ko
From: Anton Volkov @ 2017-08-08 16:19 UTC (permalink / raw)
To: samuel; +Cc: netdev, linux-kernel, ldv-project, Alexey Khoroshilov
Hello.
While searching for races in the Linux kernel I've come across
"drivers/net/irda/via-ircc.ko" module. Here are questions that I came up
with while analyzing results. Lines are given using the info from Linux
v4.12.
Consider the following case:
Thread 1: Thread 2:
via_ircc_net_open
request_irq
<turn on interrupts>
via_ircc_interrupt
-> via_ircc_dma_receive -> RxTimerHandler
(via-ircc.c: line 1488) (via-ircc.c: line 1315)
self->... = ... ... = self->...
In the via_ircc_dma_receive a lot of fields of 'self' structure are
initialized and via_ircc_interrupt with RxTimerHandler use those fields.
If no initialization happened interrupt handler and other functions that
it calls may work with incorrect data. I'm not sure how bad this case
can be and thus here are my questions. Is this situation feasible from
your point of view? If it is feasible, is it a benign race or something
serious?
Thank you for your time.
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov@ispras.ru
^ permalink raw reply
* Re: [PATCH V3 net-next 03/21] net-next/hinic: Initialize api cmd resources
From: Aviad Krawczyk @ 2017-08-08 16:23 UTC (permalink / raw)
To: David Miller
Cc: linux-kernel, netdev, bc.y, victor.gissin, zhaochen6, tony.qu
In-Reply-To: <20170803.153513.1478323340993171697.davem@davemloft.net>
Hi David,
Is it a new rule?
We can fix it, but I can look over the Linux Kernel source and there are
a lot of examples that have the same problem. I can see even code that inserted
to 4.10 with the same problem.
Thanks for your time and review,
Aviad
On 8/4/2017 1:35 AM, David Miller wrote:
> From: Aviad Krawczyk <aviad.krawczyk@huawei.com>
> Date: Thu, 3 Aug 2017 17:54:09 +0800
>
>> +static int alloc_cmd_buf(struct hinic_api_cmd_chain *chain,
>> + struct hinic_api_cmd_cell *cell, int cell_idx)
>> +{
>> + struct hinic_hwif *hwif = chain->hwif;
>> + struct pci_dev *pdev = hwif->pdev;
>> + struct hinic_api_cmd_cell_ctxt *cell_ctxt;
>> + dma_addr_t cmd_paddr;
>> + u8 *cmd_vaddr;
>> + int err = 0;
>
> Order local variables from longest to shortest line.
>
>> +static int api_cmd_create_cell(struct hinic_api_cmd_chain *chain,
>> + int cell_idx,
>> + struct hinic_api_cmd_cell *pre_node,
>> + struct hinic_api_cmd_cell **node_vaddr)
>> +{
>> + struct hinic_hwif *hwif = chain->hwif;
>> + struct pci_dev *pdev = hwif->pdev;
>> + struct hinic_api_cmd_cell_ctxt *cell_ctxt;
>> + struct hinic_api_cmd_cell *node;
>> + dma_addr_t node_paddr;
>> + int err;
>
> Likewise.
>> +static void api_cmd_destroy_cell(struct hinic_api_cmd_chain *chain,
>> + int cell_idx)
>> +{
>> + struct hinic_hwif *hwif = chain->hwif;
>> + struct pci_dev *pdev = hwif->pdev;
>> + struct hinic_api_cmd_cell_ctxt *cell_ctxt;
>> + struct hinic_api_cmd_cell *node;
>> + dma_addr_t node_paddr;
>> + size_t node_size;
>
> Likewise.
>
> etc. etc. etc.
>
> Please audit your entire submission for this problem.
>
> Thanks.
>
> .
>
^ permalink raw reply
* Re: [PATCH net-next] ibmvnic: Add netdev_dbg output for debugging
From: Stephen Hemminger @ 2017-08-08 16:27 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: netdev, jallen, tlfalcon
In-Reply-To: <20170807190258.13228.24698.stgit@ltcalpine2-lp23.aus.stglabs.ibm.com>
On Mon, 07 Aug 2017 15:02:58 -0400
Nathan Fontenot <nfont@linux.vnet.ibm.com> wrote:
> To ease debugging of the ibmvnic driver add a series of netdev_dbg()
> statements to track driver status, especially during initialization,
> removal, and resetting of the driver.
>
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Maybe use netif_dbg() and the message type stuff.
^ permalink raw reply
* Re: [PATCH v3 net-next 3/5] sock: ULP infrastructure
From: Hannes Frederic Sowa @ 2017-08-08 16:38 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, rohit, davejwatson, john.fastabend
In-Reply-To: <20170807172818.31855-4-tom@quantonium.net>
Tom Herbert <tom@quantonium.net> writes:
> +#ifdef CONFIG_MODULES
> + if (!ulp && capable(CAP_NET_ADMIN)) {
> + rcu_read_unlock();
> + request_module("%s", name);
> + rcu_read_lock();
> + ulp = ulp_find(name);
> + }
> +#endif
It looks to me that this allows users with only CAP_NET_ADMIN
privileges to load every module?
^ permalink raw reply
* Re: [PATCH net] rds: Reintroduce statistics counting
From: Santosh Shilimkar @ 2017-08-08 16:41 UTC (permalink / raw)
To: Håkon Bugge, David S . Miller
Cc: netdev, linux-rdma, rds-devel, linux-kernel, knut.omang,
wei.lin-guay
In-Reply-To: <20170808091332.5413-1-Haakon.Bugge@oracle.com>
On 8/8/2017 2:13 AM, Håkon Bugge wrote:
> In commit 7e3f2952eeb1 ("rds: don't let RDS shutdown a connection
> while senders are present"), refilling the receive queue was removed
> from rds_ib_recv(), along with the increment of
> s_ib_rx_refill_from_thread.
>
> Commit 73ce4317bf98 ("RDS: make sure we post recv buffers")
> re-introduces filling the receive queue from rds_ib_recv(), but does
> not add the statistics counter. rds_ib_recv() was later renamed to
> rds_ib_recv_path().
>
> This commit reintroduces the statistics counting of
> s_ib_rx_refill_from_thread and s_ib_rx_refill_from_cq.
>
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: Wei Lin Guay <wei.lin.guay@oracle.com>
> ---
Looks fine.
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* Re: [RFC PATCH 2/2] bpf: Initialise mod[] in bpf_trace_printk
From: David Miller @ 2017-08-08 16:48 UTC (permalink / raw)
To: daniel; +Cc: james.hogan, ast, linux-kernel, rostedt, mingo, netdev
In-Reply-To: <59897A7C.10009@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue, 08 Aug 2017 10:46:52 +0200
> On 08/08/2017 12:25 AM, James Hogan wrote:
>> In bpf_trace_printk(), the elements in mod[] are left uninitialised,
>> but
>> they are then incremented to track the width of the formats. Zero
>> initialise the array just in case the memory contains non-zero values
>> on
>> entry.
>>
>> Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call
>> bpf_trace_printk()")
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: netdev@vger.kernel.org
>> ---
>> When I checked (on MIPS32), the elements tended to have the value zero
>> anyway (does BPF zero the stack or something clever?), so this is a
>> purely theoretical fix.
>> ---
>> kernel/trace/bpf_trace.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 32dcbe1b48f2..86a52857d941 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32,
>> fmt_size, u64, arg1,
>> u64, arg2, u64, arg3)
>> {
>> bool str_seen = false;
>> - int mod[3] = {};
>> + int mod[3] = { 0, 0, 0 };
>
> I'm probably missing something, but is the behavior of gcc wrt
> above initializers different on mips (it zeroes just fine on x86
> at least)? If yes, we'd probably need a cocci script to also check
> rest of the kernel given this is used in a number of places. Hm,
> could you elaborate?
This change is not necessary at all.
An empty initializer must clear the whole object to zero.
"theoretical" fix indeed... :-(
^ permalink raw reply
* [PATCH] isdn: hisax: hfc_usb: constify usb_device_id
From: Arvind Yadav @ 2017-08-08 16:49 UTC (permalink / raw)
To: isdn; +Cc: linux-kernel, netdev
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/isdn/hisax/hfc_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/hisax/hfc_usb.c b/drivers/isdn/hisax/hfc_usb.c
index ef47480..e821218 100644
--- a/drivers/isdn/hisax/hfc_usb.c
+++ b/drivers/isdn/hisax/hfc_usb.c
@@ -65,7 +65,7 @@ typedef struct {
} hfcsusb_vdata;
/* VID/PID device list */
-static struct usb_device_id hfcusb_idtab[] = {
+static const struct usb_device_id hfcusb_idtab[] = {
{
USB_DEVICE(0x0959, 0x2bd0),
.driver_info = (unsigned long) &((hfcsusb_vdata)
--
2.7.4
^ permalink raw reply related
* [PATCH] isdn: hfcsusb: constify usb_device_id
From: Arvind Yadav @ 2017-08-08 16:49 UTC (permalink / raw)
To: isdn; +Cc: linux-kernel, netdev
In-Reply-To: <fb99bdbed78f5948e9ebb99365d002d41c8437ec.1502210664.git.arvind.yadav.cs@gmail.com>
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/isdn/hardware/mISDN/hfcsusb.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.h b/drivers/isdn/hardware/mISDN/hfcsusb.h
index 4157311..5f8f1d9 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.h
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.h
@@ -337,7 +337,7 @@ static const char *HFC_NT_LAYER1_STATES[HFC_MAX_NT_LAYER1_STATE + 1] = {
};
/* supported devices */
-static struct usb_device_id hfcsusb_idtab[] = {
+static const struct usb_device_id hfcsusb_idtab[] = {
{
USB_DEVICE(0x0959, 0x2bd0),
.driver_info = (unsigned long) &((struct hfcsusb_vdata)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH V3 net-next 03/21] net-next/hinic: Initialize api cmd resources
From: David Miller @ 2017-08-08 17:00 UTC (permalink / raw)
To: aviad.krawczyk
Cc: linux-kernel, netdev, bc.y, victor.gissin, zhaochen6, tony.qu
In-Reply-To: <912a2667-8b20-cbc2-4c32-7d34bfb6bb9b@huawei.com>
From: Aviad Krawczyk <aviad.krawczyk@huawei.com>
Date: Tue, 8 Aug 2017 19:23:32 +0300
> Is it a new rule?
It's at least 10 years old.
> We can fix it, but I can look over the Linux Kernel source and there are
> a lot of examples that have the same problem.
Stop right there.
Just because there is code in the kernel with a problem doesn't mean
you can submit new code with that problem.
^ permalink raw reply
* Re: [PATCH net] tcp: fastopen: tcp_connect() must refresh the route
From: Wei Wang @ 2017-08-08 17:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Yuchung Cheng, Dmitry Vyukov
In-Reply-To: <1502181718.4487.9.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Aug 8, 2017 at 1:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> With new TCP_FASTOPEN_CONNECT socket option, there is a possibility
> to call tcp_connect() while socket sk_dst_cache is either NULL
> or invalid.
>
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
> +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> +0 setsockopt(4, SOL_TCP, TCP_FASTOPEN_CONNECT, [1], 4) = 0
> +0 connect(4, ..., ...) = 0
>
> << sk->sk_dst_cache becomes obsolete, or even set to NULL >>
>
> +1 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
>
>
> We need to refresh the route otherwise bad things can happen,
> especially when syzkaller is running on the host :/
>
> Fixes: 19f6d3f3c8422 ("net/tcp-fastopen: Add new API support")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
Thanks a lot for the fix, Eric.
Acked-by: Wei Wang <weiwan@google.com>
^ permalink raw reply
* Re: [PATCH v3 net-next 0/5] ulp: Generalize ULP infrastructure
From: Tom Herbert @ 2017-08-08 17:04 UTC (permalink / raw)
To: John Fastabend
Cc: Tom Herbert, Linux Kernel Network Developers, Rohit Seth,
Dave Watson
In-Reply-To: <5989D958.6080506@gmail.com>
On Tue, Aug 8, 2017 at 8:31 AM, John Fastabend <john.fastabend@gmail.com> wrote:
> On 08/07/2017 10:28 AM, Tom Herbert wrote:
>> Generalize the ULP infrastructure that was recently introduced to
>> support kTLS. This adds a SO_ULP socket option and creates new fields in
>> sock structure for ULP ops and ULP data. Also, the interface allows
>> additional per ULP parameters to be set so that a ULP can be pushed
>> and operations started in one shot.
>>
>> In this patch set:
>> - Minor dependency fix in inet_common.h
>> - Implement ULP infrastructure as a socket mechanism
>> - Fixes TCP and TLS to use the new method (maintaining backwards
>> API compatibility)
>> - Adds a ulp.txt document
>>
>> Tested: Ran simple ULP. Dave Watson verified kTLS works.
>>
>> -v2: Fix compilation errors when CONFIG_ULP_SOCK not set.
>> -v3: Fix one more build issue, check that sk_protocol is IPPROTO_TCP
>> in tsl_init. Also, fix a couple of minor issues related to
>> introducing locked versions of sendmsg, send page. Thanks to
>> Dave Watson, John Fastabend, and Mat Martineau for testing and
>> providing fixes.
>>
>
>
> Hi Tom, Dave,
>
> I'm concerned about the performance impact of walking a list and
> doing string compares on every socket we create with kTLS. Dave
> do you have any request/response tests for kTLS that would put pressure
> on the create/destroy time of this infrastructure? We should do some
> tests with dummy entries in the ULP list to understand the impact of
> this list walk.
>
> I like the underlying TCP generalized hooks, but do we really expect a
> lot of these hooks to exist? If we only have two on the roadmap
> (kTLS and socktap) it seems a bit overkill. Further, if we really expect
> many ULP objects then the list walk and compare will become more expensive
> perhaps becoming noticeable in request per second metrics.
>
> Why not just create another socktap socketopt? That will be better from
> complexity and likely performance sides.
>
IMO, given that there is at most two even proposed at this point I
don't there's much point addressing performance. When ULP feature
catches on and we start see a whole bunch of them then it's
straightforward to use a hash table or some more efficient mechanism.
Tom
> Thanks,
> .John
>
^ permalink raw reply
* Re: [PATCH v3 net-next 3/5] sock: ULP infrastructure
From: Tom Herbert @ 2017-08-08 17:07 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Tom Herbert, Linux Kernel Network Developers, Rohit Seth,
Dave Watson, john fastabend
In-Reply-To: <87shh2t3od.fsf@stressinduktion.org>
On Tue, Aug 8, 2017 at 9:38 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Tom Herbert <tom@quantonium.net> writes:
>
>> +#ifdef CONFIG_MODULES
>> + if (!ulp && capable(CAP_NET_ADMIN)) {
>> + rcu_read_unlock();
>> + request_module("%s", name);
>> + rcu_read_lock();
>> + ulp = ulp_find(name);
>> + }
>> +#endif
>
> It looks to me that this allows users with only CAP_NET_ADMIN
> privileges to load every module?
It's a carryover. Probably should remove the check.
Tom
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: Yuchung Cheng @ 2017-08-08 17:25 UTC (permalink / raw)
To: Rao Shoaib; +Cc: David Miller, Alexey Kuznetsov, netdev, Jerry Chu
In-Reply-To: <20170807181614.GA16700@caduceus5>
On Mon, Aug 7, 2017 at 11:16 AM, Rao Shoaib <rao.shoaib@oracle.com> wrote:
> Change from version 0: Rationale behind the change:
>
> The man page for tcp(7) states
>
> when used with the TCP keepalive (SO_KEEPALIVE) option, TCP_USER_TIMEOUT will
> override keepalive to determine when to close a connection due to keepalive
> failure.
>
> This is ambigious at best. user expectation is most likely that the connection
> will be reset after TCP_USER_TIMEOUT milliseconds of inactivity.
ccing the original author Jerry Chu who can tell more.
>
> The code however waits for the keepalive to kick-in (default 2hrs) and than
> after one failure resets the conenction.
>
> What is the rationale for that ? The same effect can be obtained by simply
> changing the value of tcp_keep_alive_probes.
>
> Since the TCP_USER_TIMEOUT option was added based on RFC 5482 we need to follow
> the RFC. Which states
>
> 4.2 TCP keep-Alives:
> Some TCP implementations, such as those in BSD systems, use a
> different abort policy for TCP keep-alives than for user data. Thus,
> the TCP keep-alive mechanism might abort a connection that would
> otherwise have survived the transient period without connectivity.
> Therefore, if a connection that enables keep-alives is also using the
> TCP User Timeout Option, then the keep-alive timer MUST be set to a
> value larger than that of the adopted USER TIMEOUT.
>
> This patch enforces the MUST and also dis-associates user timeout from keep
> alive. A man page patch will be submitted separately.
>
> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
> ---
> net/ipv4/tcp.c | 10 ++++++++--
> net/ipv4/tcp_timer.c | 9 +--------
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 71ce33d..f2af44d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2628,7 +2628,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> break;
>
> case TCP_KEEPIDLE:
> - if (val < 1 || val > MAX_TCP_KEEPIDLE)
> + /* Per RFC5482 keepalive_time must be > user_timeout */
> + if (val < 1 || val > MAX_TCP_KEEPIDLE ||
> + ((val * HZ) <= icsk->icsk_user_timeout))
> err = -EINVAL;
> else {
> tp->keepalive_time = val * HZ;
> @@ -2724,8 +2726,12 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> case TCP_USER_TIMEOUT:
> /* Cap the max time in ms TCP will retry or probe the window
> * before giving up and aborting (ETIMEDOUT) a connection.
> + * Per RFC5482 TCP user timeout must be < keepalive_time.
> + * If the default value changes later -- all bets are off.
> */
> - if (val < 0)
> + if (val < 0 || (tp->keepalive_time &&
> + tp->keepalive_time <= msecs_to_jiffies(val)) ||
> + net->ipv4.sysctl_tcp_keepalive_time <= msecs_to_jiffies(val))
> err = -EINVAL;
> else
> icsk->icsk_user_timeout = msecs_to_jiffies(val);
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index c0feeee..d39fe60 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -664,14 +664,7 @@ static void tcp_keepalive_timer (unsigned long data)
> elapsed = keepalive_time_elapsed(tp);
>
> if (elapsed >= keepalive_time_when(tp)) {
> - /* If the TCP_USER_TIMEOUT option is enabled, use that
> - * to determine when to timeout instead.
> - */
> - if ((icsk->icsk_user_timeout != 0 &&
> - elapsed >= icsk->icsk_user_timeout &&
> - icsk->icsk_probes_out > 0) ||
> - (icsk->icsk_user_timeout == 0 &&
> - icsk->icsk_probes_out >= keepalive_probes(tp))) {
> + if (icsk->icsk_probes_out >= keepalive_probes(tp)) {
> tcp_send_active_reset(sk, GFP_ATOMIC);
> tcp_write_err(sk);
> goto out;
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH net-next] ibmvnic: Add netdev_dbg output for debugging
From: Nathan Fontenot @ 2017-08-08 17:27 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, jallen, tlfalcon
In-Reply-To: <20170808092714.5f15a1bf@xeon-e3>
On 08/08/2017 11:27 AM, Stephen Hemminger wrote:
> On Mon, 07 Aug 2017 15:02:58 -0400
> Nathan Fontenot <nfont@linux.vnet.ibm.com> wrote:
>
>> To ease debugging of the ibmvnic driver add a series of netdev_dbg()
>> statements to track driver status, especially during initialization,
>> removal, and resetting of the driver.
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>
> Maybe use netif_dbg() and the message type stuff.
Oh! I like this even more. This just begs to update all of the message
reporting in the driver though.
Let's scrap this patch. I'll work on a new patchset based on using
the netif_*() infrastructure.
-Nathan
^ permalink raw reply
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
From: Willem de Bruijn @ 2017-08-08 17:46 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
In-Reply-To: <CAF=yD-Ls_brMMJzekHw1Q7kMaa+2e3t1=c=_wCeWabxfcKW3Sw@mail.gmail.com>
>> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct
>> sk_buff *skb, bool tx_path)
>> {
>> if (tx_path)
>> return skb->ip_summed != CHECKSUM_PARTIAL &&
>> + skb->ip_summed != CHECKSUM_UNNECESSARY &&
>> skb->ip_summed != CHECKSUM_NONE;
>
> Good catch. Only, the CHECKSUM_NONE case was added specifically to
> work around this UFO issue on the tx path in commit 6e7bc478c9a0
> ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change
> the value generated by UFO, we can remove that statement, so
>
> + skb->ip_summed != CHECKSUM_UNNECESSARY;
> - skb->ip_summed != CHECKSUM_NONE;
>
> Else the entire check becomes a NOOP. These are the only three valid
> states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY
> to begin with, it arguably already is practically a NOOP. I need to
> look more closely what the statement is intended to protect against,
> before we relax it even further.
On transmit, packets entering skb_gso_segment are expected to always
have ip_summed CHECKSUM_PARTIAL. This check was added to track down
unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to
track down GSO checksum bug").
Only when called for the second time, after skb_mac_gso_segment, do we
have to possibly handle the case where the GSO layer computes the
checksum and changes ip_summed.
Since this only goes into 4.11 to 4.13, making two separate
skb_needs_check variants for these two call sites seems overkill. I
will send the simple fix to convert CHECKSUM_NONE to
CHECKSUM_UNNECESSARY.
As a side effect of removing UFO in 4.14-rc1, we can also revert
commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for
tx") in net-next.
^ permalink raw reply
* Re: [PATCH net] tcp: fastopen: tcp_connect() must refresh the route
From: Yuchung Cheng @ 2017-08-08 17:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Wei Wang, Dmitry Vyukov
In-Reply-To: <1502181718.4487.9.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Aug 8, 2017 at 1:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> With new TCP_FASTOPEN_CONNECT socket option, there is a possibility
> to call tcp_connect() while socket sk_dst_cache is either NULL
> or invalid.
>
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
> +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> +0 setsockopt(4, SOL_TCP, TCP_FASTOPEN_CONNECT, [1], 4) = 0
> +0 connect(4, ..., ...) = 0
>
> << sk->sk_dst_cache becomes obsolete, or even set to NULL >>
>
> +1 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
>
>
> We need to refresh the route otherwise bad things can happen,
> especially when syzkaller is running on the host :/
>
> Fixes: 19f6d3f3c8422 ("net/tcp-fastopen: Add new API support")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
Acked-by: Yuchung Cheng <ycheng@google.com>
Thanks for the fix!
> net/ipv4/tcp_output.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 276406a83a37..b7661a68d498 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3436,6 +3436,10 @@ int tcp_connect(struct sock *sk)
> int err;
>
> tcp_call_bpf(sk, BPF_SOCK_OPS_TCP_CONNECT_CB);
> +
> + if (inet_csk(sk)->icsk_af_ops->rebuild_header(sk))
> + return -EHOSTUNREACH; /* Routing failure or similar. */
> +
> tcp_connect_init(sk);
>
> if (unlikely(tp->repair)) {
>
>
^ permalink raw reply
* [PATCH net] net: avoid skb_warn_bad_offload false positives on UFO
From: Willem de Bruijn @ 2017-08-08 18:22 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, xiangxia.m.yue, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
skb_warn_bad_offload triggers a warning when an skb enters the GSO
stack at __skb_gso_segment that does not have CHECKSUM_PARTIAL
checksum offload set.
Commit b2504a5dbef3 ("net: reduce skb_warn_bad_offload() noise")
observed that SKB_GSO_DODGY producers can trigger the check and
that passing those packets through the GSO handlers will fix it
up. But, the software UFO handler will set ip_summed to
CHECKSUM_NONE.
When __skb_gso_segment is called from the receive path, this
triggers the warning again.
Make UFO set CHECKSUM_UNNECESSARY instead of CHECKSUM_NONE. On
Tx these two are equivalent. On Rx, this better matches the
skb state (checksum computed), as CHECKSUM_NONE here means no
checksum computed.
See also this thread for context:
http://patchwork.ozlabs.org/patch/799015/
Fixes: b2504a5dbef3 ("net: reduce skb_warn_bad_offload() noise")
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/core/dev.c | 2 +-
net/ipv4/udp_offload.c | 2 +-
net/ipv6/udp_offload.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 8515f8fe0460..ce15a06d5558 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2739,7 +2739,7 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
{
if (tx_path)
return skb->ip_summed != CHECKSUM_PARTIAL &&
- skb->ip_summed != CHECKSUM_NONE;
+ skb->ip_summed != CHECKSUM_UNNECESSARY;
return skb->ip_summed == CHECKSUM_NONE;
}
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 781250151d40..0932c85b42af 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
if (uh->check == 0)
uh->check = CSUM_MANGLED_0;
- skb->ip_summed = CHECKSUM_NONE;
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
/* If there is no outer header we can fake a checksum offload
* due to the fact that we have already done the checksum in
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index a2267f80febb..e7d378c032cb 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -72,7 +72,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
if (uh->check == 0)
uh->check = CSUM_MANGLED_0;
- skb->ip_summed = CHECKSUM_NONE;
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
/* If there is no outer header we can fake a checksum offload
* due to the fact that we have already done the checksum in
--
2.14.0.rc1.383.gd1ce394fe2-goog
^ permalink raw reply related
* [PATCH net-next v2] net: ipv6: avoid overhead when no custom FIB rules are installed
From: Vincent Bernat @ 2017-08-08 18:23 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jiri Pirko,
netdev, David Ahern
Cc: Vincent Bernat
In-Reply-To: <66e3d00a-6788-2e86-9563-fc1424a6a250@gmail.com>
If the user hasn't installed any custom rules, don't go through the
whole FIB rules layer. This is pretty similar to f4530fa574df (ipv4:
Avoid overhead when no custom FIB rules are installed).
Using a micro-benchmark module [1], timing ip6_route_output() with
get_cycles(), with 40,000 routes in the main routing table, before this
patch:
min=606 max=12911 count=627 average=1959 95th=4903 90th=3747 50th=1602 mad=821
table=254 avgdepth=21.8 maxdepth=39
value │ ┊ count
600 │▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒ 199
880 │▒▒▒░░░░░░░░░░░░░░░░ 43
1160 │▒▒▒░░░░░░░░░░░░░░░░░░░░ 48
1440 │▒▒▒░░░░░░░░░░░░░░░░░░░░░░░ 43
1720 │▒▒▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░ 59
2000 │▒▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 50
2280 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 26
2560 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 31
2840 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 28
3120 │▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 17
3400 │▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 17
3680 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 8
3960 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 11
4240 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 6
4520 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 6
4800 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 9
After:
min=544 max=11687 count=627 average=1776 95th=4546 90th=3585 50th=1227 mad=565
table=254 avgdepth=21.8 maxdepth=39
value │ ┊ count
540 │▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒ 201
800 │▒▒▒▒▒░░░░░░░░░░░░░░░░ 63
1060 │▒▒▒▒▒░░░░░░░░░░░░░░░░░░░░░ 68
1320 │▒▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░ 39
1580 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 32
1840 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 32
2100 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 34
2360 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 33
2620 │▒▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 26
2880 │▒░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 22
3140 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 9
3400 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 8
3660 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 9
3920 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 8
4180 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 8
4440 │░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 8
At the frequency of the host during the bench (~ 3.7 GHz), this is
about a 100 ns difference on the median value.
A next step would be to collapse local and main tables, as in
0ddcf43d5d4a (ipv4: FIB Local/MAIN table collapse).
[1]: https://github.com/vincentbernat/network-lab/blob/master/lab-routes-ipv6/kbench_mod.c
Signed-off-by: Vincent Bernat <vincent@bernat.im>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/netns/ipv6.h | 1 +
net/ipv6/fib6_rules.c | 40 +++++++++++++++++++++++++++-------------
net/ipv6/route.c | 1 +
3 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index abdf3b40303b..0e50bf3ed097 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -65,6 +65,7 @@ struct netns_ipv6 {
unsigned int ip6_rt_gc_expire;
unsigned long ip6_rt_last_gc;
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+ bool fib6_has_custom_rules;
struct rt6_info *ip6_prohibit_entry;
struct rt6_info *ip6_blk_hole_entry;
struct fib6_table *fib6_local_tbl;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 2f29e4e33bd3..b240f24a6e52 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -63,19 +63,32 @@ unsigned int fib6_rules_seq_read(struct net *net)
struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
int flags, pol_lookup_t lookup)
{
- struct fib_lookup_arg arg = {
- .lookup_ptr = lookup,
- .flags = FIB_LOOKUP_NOREF,
- };
-
- /* update flow if oif or iif point to device enslaved to l3mdev */
- l3mdev_update_flow(net, flowi6_to_flowi(fl6));
-
- fib_rules_lookup(net->ipv6.fib6_rules_ops,
- flowi6_to_flowi(fl6), flags, &arg);
-
- if (arg.result)
- return arg.result;
+ if (net->ipv6.fib6_has_custom_rules) {
+ struct fib_lookup_arg arg = {
+ .lookup_ptr = lookup,
+ .flags = FIB_LOOKUP_NOREF,
+ };
+
+ /* update flow if oif or iif point to device enslaved to l3mdev */
+ l3mdev_update_flow(net, flowi6_to_flowi(fl6));
+
+ fib_rules_lookup(net->ipv6.fib6_rules_ops,
+ flowi6_to_flowi(fl6), flags, &arg);
+
+ if (arg.result)
+ return arg.result;
+ } else {
+ struct rt6_info *rt;
+
+ rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, flags);
+ if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
+ return &rt->dst;
+ ip6_rt_put(rt);
+ rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, flags);
+ if (rt->dst.error != -EAGAIN)
+ return &rt->dst;
+ ip6_rt_put(rt);
+ }
dst_hold(&net->ipv6.ip6_null_entry->dst);
return &net->ipv6.ip6_null_entry->dst;
@@ -245,6 +258,7 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
rule6->dst.plen = frh->dst_len;
rule6->tclass = frh->tos;
+ net->ipv6.fib6_has_custom_rules = true;
err = 0;
errout:
return err;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index aba07fce67fb..7ecbe5eb19f8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3934,6 +3934,7 @@ static int __net_init ip6_route_net_init(struct net *net)
ip6_template_metrics, true);
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+ net->ipv6.fib6_has_custom_rules = false;
net->ipv6.ip6_prohibit_entry = kmemdup(&ip6_prohibit_entry_template,
sizeof(*net->ipv6.ip6_prohibit_entry),
GFP_KERNEL);
--
2.13.3
^ permalink raw reply related
* Re: sysctl, argument parsing, possible bug
From: Massimo Sala @ 2017-08-08 18:26 UTC (permalink / raw)
To: LKML, Linux Kernel Network Developers
I make another test with kernel 4.9.32-15.41
sysctl procps version 3.2.8
sysctl net.ipv4.conf.eth0.100.forwarding
error: "net.ipv4.conf.eth0.100.forwarding" is an unknown key
so I install busybox :
BusyBox v1.19.3
busybox sysctl net.ipv4.conf.eth0.100.forwarding
net.ipv4.conf.eth0.100.forwarding = 0
It is working, as I expect reading busybox source sysctl.c
Stephen, I test
sysctl net/ipv4/conf/eth0.100/forwarding
I confirm it works.
What is the problem ?
As sysctl, also automation tools and scripts cannot be "netdev names
aware", and so they fail using the usual dot notation.
I don't pretend to change sysctl to read from the /proc/sys/
directory, as busybox does.
I suggest to add a remark to the man page of sysctl, reporting the
difference between the two tools and an example of the alternate
syntax :
sysctl net/ipv4/conf/eth0.100/forwarding
Thank you for your attention.
Best regards, Massimo
^ permalink raw reply
* Re: [PATCH 00/35] constify net usb_device_id
From: Kalle Valo @ 2017-08-08 18:36 UTC (permalink / raw)
To: Arvind Yadav; +Cc: wg, mkl, linux-can, netdev, linux-kernel
In-Reply-To: <1502207730-11404-1-git-send-email-arvind.yadav.cs@gmail.com>
Arvind Yadav <arvind.yadav.cs@gmail.com> writes:
> usb_device_id are not supposed to change at runtime. All functions
> working with usb_device_id provided by <linux/usb.h> work with
> const usb_device_id. So mark the non-const structs as const.
[...]
> [PATCH 16/35] wireless: ath: ar5523: constify usb_device_id
> [PATCH 17/35] wireless: ath: ath6kl: constify usb_device_id
> [PATCH 18/35] wireless: ath: ath9k: constify usb_device_id
> [PATCH 19/35] wireless: ath: carl9170: constify usb_device_id
> [PATCH 20/35] wireless: atmel: at76c50x: constify usb_device_id
> [PATCH 21/35] wireless: broadcom: brcm80211: constify usb_device_id
> [PATCH 22/35] wireless: intersil: orinoco: constify usb_device_id
> [PATCH 23/35] wireless: intersil: p54: constify usb_device_id
> [PATCH 24/35] wireless: marvell: libertas: constify usb_device_id
> [PATCH 25/35] wireless: marvell: libertas_tf: constify usb_device_id
> [PATCH 26/35] wireless: marvell: mwifiex: constify usb_device_id
> [PATCH 27/35] wireless: mediatek: mt7601u: constify usb_device_id
> [PATCH 28/35] wireless: ralink: rt2500usb: constify usb_device_id
> [PATCH 29/35] wireless: ralink: rt2800usb: constify usb_device_id
> [PATCH 30/35] wireless: ralink: rt73usb: constify usb_device_id
> [PATCH 31/35] wireless: realtek: rtl8187: constify usb_device_id
> [PATCH 32/35] wireless: realtek: rtl8xxxu: constify usb_device_id
> [PATCH 33/35] wireless: realtek: rtl8192cu: constify usb_device_id
> [PATCH 34/35] wireless: zydas: zd1201: constify usb_device_id
> [PATCH 35/35] wireless: zydas: zd1211rw: constify usb_device_id
No need to put the whole path to the title, it's enough to have the name
of the driver there. For example:
[PATCH 16/35] ar5523: constify usb_device_id
[PATCH 17/35] ath6kl: constify usb_device_id
[PATCH 18/35] ath9k: constify usb_device_id
Please resubmit the wireless patches separately in their own patchset.
--
Kalle Valo
^ permalink raw reply
* Re: sysctl, argument parsing, possible bug
From: Stephen Hemminger @ 2017-08-08 19:04 UTC (permalink / raw)
To: Massimo Sala; +Cc: LKML, Linux Kernel Network Developers
In-Reply-To: <CAKv20-XwH=Ltqt6Xw++q2LbcHKmJhn76ZukF-Tr6a4Yd6rbJAg@mail.gmail.com>
On Tue, 8 Aug 2017 20:26:36 +0200
Massimo Sala <massimo.sala.71@gmail.com> wrote:
> I make another test with kernel 4.9.32-15.41
>
> sysctl procps version 3.2.8
>
> sysctl net.ipv4.conf.eth0.100.forwarding
> error: "net.ipv4.conf.eth0.100.forwarding" is an unknown key
>
>
> so I install busybox :
>
> BusyBox v1.19.3
>
> busybox sysctl net.ipv4.conf.eth0.100.forwarding
> net.ipv4.conf.eth0.100.forwarding = 0
>
> It is working, as I expect reading busybox source sysctl.c
>
>
>
> Stephen, I test
> sysctl net/ipv4/conf/eth0.100/forwarding
> I confirm it works.
>
>
> What is the problem ?
>
> As sysctl, also automation tools and scripts cannot be "netdev names
> aware", and so they fail using the usual dot notation.
>
>
> I don't pretend to change sysctl to read from the /proc/sys/
> directory, as busybox does.
>
> I suggest to add a remark to the man page of sysctl, reporting the
> difference between the two tools and an example of the alternate
> syntax :
> sysctl net/ipv4/conf/eth0.100/forwarding
>
>
> Thank you for your attention.
> Best regards, Massimo
Busybox has always been a restricted subset of the upstream standard tools.
If you have problems with busybox take it up with those developers directly;
this is not the right mailing list for that.
^ permalink raw reply
* Re: [PATCH net-next v2] net: ipv6: avoid overhead when no custom FIB rules are installed
From: David Ahern @ 2017-08-08 19:10 UTC (permalink / raw)
To: Vincent Bernat, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Jiri Pirko, netdev
In-Reply-To: <20170808182349.4987-1-vincent@bernat.im>
On 8/8/17 12:23 PM, Vincent Bernat wrote:
> If the user hasn't installed any custom rules, don't go through the
> whole FIB rules layer. This is pretty similar to f4530fa574df (ipv4:
> Avoid overhead when no custom FIB rules are installed).
>
> Using a micro-benchmark module [1], timing ip6_route_output() with
> get_cycles(), with 40,000 routes in the main routing table, before this
> patch:
...
> At the frequency of the host during the bench (~ 3.7 GHz), this is
> about a 100 ns difference on the median value.
>
> A next step would be to collapse local and main tables, as in
> 0ddcf43d5d4a (ipv4: FIB Local/MAIN table collapse).
>
> [1]: https://github.com/vincentbernat/network-lab/blob/master/lab-routes-ipv6/kbench_mod.c
>
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> ---
> include/net/netns/ipv6.h | 1 +
> net/ipv6/fib6_rules.c | 40 +++++++++++++++++++++++++++-------------
> net/ipv6/route.c | 1 +
> 3 files changed, 29 insertions(+), 13 deletions(-)
>
LGTM.
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* [PATCH net-next v2] wan: dscc4: add checks for dma mapping errors
From: Alexey Khoroshilov @ 2017-08-08 19:28 UTC (permalink / raw)
To: Francois Romieu, David S . Miller
Cc: Alexey Khoroshilov, netdev, linux-kernel, ldv-project
In-Reply-To: <20170807215943.GA22433@electric-eye.fr.zoreil.com>
The driver does not check if mapping dma memory succeed.
The patch adds the checks and failure handling.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v2: Fix issues noted by David Miller and Francois Romieu.
drivers/net/wan/dscc4.c | 52 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
index 799830f..6a9ffac 100644
--- a/drivers/net/wan/dscc4.c
+++ b/drivers/net/wan/dscc4.c
@@ -518,23 +518,31 @@ static void dscc4_release_ring(struct dscc4_dev_priv *dpriv)
static inline int try_get_rx_skb(struct dscc4_dev_priv *dpriv,
struct net_device *dev)
{
+ struct pci_dev *pdev = dpriv->pci_priv->pdev;
unsigned int dirty = dpriv->rx_dirty%RX_RING_SIZE;
struct RxFD *rx_fd = dpriv->rx_fd + dirty;
const int len = RX_MAX(HDLC_MAX_MRU);
struct sk_buff *skb;
- int ret = 0;
+ dma_addr_t addr;
skb = dev_alloc_skb(len);
+ if (!skb)
+ goto err_out;
+
+ skb->protocol = hdlc_type_trans(skb, dev);
+ addr = pci_map_single(pdev, skb->data, len, PCI_DMA_FROMDEVICE);
+ if (pci_dma_mapping_error(pdev, addr))
+ goto err_free_skb;
+
dpriv->rx_skbuff[dirty] = skb;
- if (skb) {
- skb->protocol = hdlc_type_trans(skb, dev);
- rx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
- skb->data, len, PCI_DMA_FROMDEVICE));
- } else {
- rx_fd->data = 0;
- ret = -1;
- }
- return ret;
+ rx_fd->data = cpu_to_le32(addr);
+ return 0;
+
+err_free_skb:
+ dev_kfree_skb_any(skb);
+err_out:
+ rx_fd->data = 0;
+ return -1;
}
/*
@@ -1147,14 +1155,22 @@ static netdev_tx_t dscc4_start_xmit(struct sk_buff *skb,
struct dscc4_dev_priv *dpriv = dscc4_priv(dev);
struct dscc4_pci_priv *ppriv = dpriv->pci_priv;
struct TxFD *tx_fd;
+ dma_addr_t addr;
int next;
+ addr = pci_map_single(ppriv->pdev, skb->data, skb->len,
+ PCI_DMA_TODEVICE);
+ if (pci_dma_mapping_error(ppriv->pdev, addr)) {
+ dev_kfree_skb_any(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
+
next = dpriv->tx_current%TX_RING_SIZE;
dpriv->tx_skbuff[next] = skb;
tx_fd = dpriv->tx_fd + next;
tx_fd->state = FrameEnd | TO_STATE_TX(skb->len);
- tx_fd->data = cpu_to_le32(pci_map_single(ppriv->pdev, skb->data, skb->len,
- PCI_DMA_TODEVICE));
+ tx_fd->data = cpu_to_le32(addr);
tx_fd->complete = 0x00000000;
tx_fd->jiffies = jiffies;
mb();
@@ -1887,16 +1903,22 @@ static struct sk_buff *dscc4_init_dummy_skb(struct dscc4_dev_priv *dpriv)
skb = dev_alloc_skb(DUMMY_SKB_SIZE);
if (skb) {
+ struct pci_dev *pdev = dpriv->pci_priv->pdev;
int last = dpriv->tx_dirty%TX_RING_SIZE;
struct TxFD *tx_fd = dpriv->tx_fd + last;
+ dma_addr_t addr;
skb->len = DUMMY_SKB_SIZE;
skb_copy_to_linear_data(skb, version,
strlen(version) % DUMMY_SKB_SIZE);
tx_fd->state = FrameEnd | TO_STATE_TX(DUMMY_SKB_SIZE);
- tx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
- skb->data, DUMMY_SKB_SIZE,
- PCI_DMA_TODEVICE));
+ addr = pci_map_single(pdev, skb->data, DUMMY_SKB_SIZE,
+ PCI_DMA_TODEVICE);
+ if (pci_dma_mapping_error(pdev, addr)) {
+ dev_kfree_skb_any(skb);
+ return NULL;
+ }
+ tx_fd->data = cpu_to_le32(addr);
dpriv->tx_skbuff[last] = skb;
}
return skb;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 net-next 0/5] ulp: Generalize ULP infrastructure
From: John Fastabend @ 2017-08-08 19:30 UTC (permalink / raw)
To: Tom Herbert
Cc: Tom Herbert, Linux Kernel Network Developers, Rohit Seth,
Dave Watson
In-Reply-To: <CALx6S37HW6VF1ed5RQx7HCNMGM8pRpb54R1OSFHN8mDNpsc1uQ@mail.gmail.com>
On 08/08/2017 10:04 AM, Tom Herbert wrote:
> On Tue, Aug 8, 2017 at 8:31 AM, John Fastabend <john.fastabend@gmail.com> wrote:
>> On 08/07/2017 10:28 AM, Tom Herbert wrote:
>>> Generalize the ULP infrastructure that was recently introduced to
>>> support kTLS. This adds a SO_ULP socket option and creates new fields in
>>> sock structure for ULP ops and ULP data. Also, the interface allows
>>> additional per ULP parameters to be set so that a ULP can be pushed
>>> and operations started in one shot.
>>>
>>> In this patch set:
>>> - Minor dependency fix in inet_common.h
>>> - Implement ULP infrastructure as a socket mechanism
>>> - Fixes TCP and TLS to use the new method (maintaining backwards
>>> API compatibility)
>>> - Adds a ulp.txt document
>>>
>>> Tested: Ran simple ULP. Dave Watson verified kTLS works.
>>>
>>> -v2: Fix compilation errors when CONFIG_ULP_SOCK not set.
>>> -v3: Fix one more build issue, check that sk_protocol is IPPROTO_TCP
>>> in tsl_init. Also, fix a couple of minor issues related to
>>> introducing locked versions of sendmsg, send page. Thanks to
>>> Dave Watson, John Fastabend, and Mat Martineau for testing and
>>> providing fixes.
>>>
>>
>>
>> Hi Tom, Dave,
>>
>> I'm concerned about the performance impact of walking a list and
>> doing string compares on every socket we create with kTLS. Dave
>> do you have any request/response tests for kTLS that would put pressure
>> on the create/destroy time of this infrastructure? We should do some
>> tests with dummy entries in the ULP list to understand the impact of
>> this list walk.
>>
>> I like the underlying TCP generalized hooks, but do we really expect a
>> lot of these hooks to exist? If we only have two on the roadmap
>> (kTLS and socktap) it seems a bit overkill. Further, if we really expect
>> many ULP objects then the list walk and compare will become more expensive
>> perhaps becoming noticeable in request per second metrics.
>>
>> Why not just create another socktap socketopt? That will be better from
>> complexity and likely performance sides.
>>
> IMO, given that there is at most two even proposed at this point I
> don't there's much point addressing performance. When ULP feature
> catches on and we start see a whole bunch of them then it's
> straightforward to use a hash table or some more efficient mechanism.
>
OTOH these optimizations are usually easiest to do at the beginning. And
building an enum of ULP types would allow removing string comparisons and
to do simpler unsigned comparisons. I wont complain too much here though
because this series didn't introduce the lists.
> Tom
>
>> Thanks,
>> .John
>>
^ permalink raw reply
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