* Re: [PATCH net-next] vhost_net: add a missing error return
From: Jason Wang @ 2018-09-21 2:29 UTC (permalink / raw)
To: Dan Carpenter, Michael S. Tsirkin
Cc: kvm, virtualization, netdev, kernel-janitors
In-Reply-To: <20180920100158.GA9551@mwanda>
On 2018年09月20日 18:01, Dan Carpenter wrote:
> We accidentally left out this error return so it leads to some use after
> free bugs later on.
>
> Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index dd4e0a301635..1bff6bc8161a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1244,6 +1244,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> kfree(vqs);
> kvfree(n);
> kfree(queue);
> + return -ENOMEM;
> }
> n->vqs[VHOST_NET_VQ_TX].xdp = xdp;
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks!
^ permalink raw reply
* RE: [PATCH 00/21] SMMU enablement for NXP LS1043A and LS1046A
From: Laurentiu Tudor @ 2018-09-21 7:32 UTC (permalink / raw)
To: Leo Li
Cc: robin.murphy@arm.com,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Netdev, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Madalin-cristian Bucur, Roy Pledge, Shawn Guo, David Miller
In-Reply-To: <CADRPPNTot7U6vARqtrRrw4XqhDQPSXP1QO9ZbGpNQByJPcU37Q@mail.gmail.com>
> -----Original Message-----
> From: Li Yang [mailto:leoyang.li@nxp.com]
> Sent: Thursday, September 20, 2018 10:07 PM
>
> On Thu, Sep 20, 2018 at 5:39 AM Laurentiu Tudor <laurentiu.tudor@nxp.com>
> wrote:
> >
> >
> >
> > On 19.09.2018 17:37, Robin Murphy wrote:
> > > On 19/09/18 15:18, Laurentiu Tudor wrote:
> > >> Hi Robin,
> > >>
> > >> On 19.09.2018 16:25, Robin Murphy wrote:
> > >>> Hi Laurentiu,
> > >>>
> > >>> On 19/09/18 13:35, laurentiu.tudor@nxp.com wrote:
> > >>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > >>>>
> > >>>> This patch series adds SMMU support for NXP LS1043A and LS1046A
> chips
> > >>>> and consists mostly in important driver fixes and the required
> device
> > >>>> tree updates. It touches several subsystems and consists of three
> main
> > >>>> parts:
> > >>>> - changes in soc/drivers/fsl/qbman drivers adding iommu mapping
> of
> > >>>> reserved memory areas, fixes and defered probe support
> > >>>> - changes in drivers/net/ethernet/freescale/dpaa_eth drivers
> > >>>> consisting in misc dma mapping related fixes and probe
> ordering
> > >>>> - addition of the actual arm smmu device tree node together with
> > >>>> various adjustments to the device trees
> > >>>>
> > >>>> Performance impact
> > >>>>
> > >>>> Running iperf benchmarks in a back-to-back setup (both sides
> > >>>> having smmu enabled) on a 10GBps port show an important
> > >>>> networking performance degradation of around %40 (9.48Gbps
> > >>>> linerate vs 5.45Gbps). If you need performance but without
> > >>>> SMMU support you can use "iommu.passthrough=1" to disable
> > >>>> SMMU.
> > >>>>
> > >>>> USB issue and workaround
> > >>>>
> > >>>> There's a problem with the usb controllers in these chips
> > >>>> generating smaller, 40-bit wide dma addresses instead of the
> > >>>> 48-bit
> > >>>> supported at the smmu input. So you end up in a situation
> > >>>> where the
> > >>>> smmu is mapped with 48-bit address translations, but the
> device
> > >>>> generates transactions with clipped 40-bit addresses, thus
> smmu
> > >>>> context faults are triggered. I encountered a similar
> > >>>> situation for
> > >>>> mmc that I managed to fix in software [1] however for USB I
> > >>>> did not
> > >>>> find a proper place in the code to add a similar fix. The
> only
> > >>>> workaround I found was to add this kernel parameter which
> > >>>> limits the
> > >>>> usb dma to 32-bit size: "xhci-hcd.quirks=0x800000".
> > >>>> This workaround if far from ideal, so any suggestions for a
> code
> > >>>> based workaround in this area would be greatly appreciated.
> > >>>
> > >>> If you have a nominally-64-bit device with a
> > >>> narrower-than-the-main-interconnect link in front of it, that should
> > >>> already be fixed in 4.19-rc by bus_dma_mask picking up DT dma-
> ranges,
> > >>> provided the interconnect hierarchy can be described appropriately
> (or
> > >>> at least massaged sufficiently to satisfy the binding), e.g.:
> > >>>
> > >>> / {
> > >>> ...
> > >>>
> > >>> soc {
> > >>> ranges;
> > >>> dma-ranges = <0 0 10000 0>;
> > >>>
> > >>> dev_48bit { ... };
> > >>>
> > >>> periph_bus {
> > >>> ranges;
> > >>> dma-ranges = <0 0 100 0>;
> > >>>
> > >>> dev_40bit { ... };
> > >>> };
> > >>> };
> > >>> };
> > >>>
> > >>> and if that fails to work as expected (except for PCI hosts where
> > >>> handling dma-ranges properly still needs sorting out), please do let
> us
> > >>> know ;)
> > >>>
> > >>
> > >> Just to confirm, Is this [1] the change I was supposed to test?
> > >
> > > Not quite - dma-ranges is only valid for nodes representing a bus, so
> > > putting it directly in the USB device nodes doesn't work (FWIW that's
> > > why PCI is broken, because the parser doesn't expect the
> > > bus-as-leaf-node case). That's teh point of that intermediate simple-
> bus
> > > node represented by "periph_bus" in my example (sorry, I should have
> put
> > > compatibles in to make it clearer) - often that's actually true to
> life
> > > (i.e. "soc" is something like a CCI and "periph_bus" is something like
> > > an AXI NIC gluing a bunch of lower-bandwidth DMA masters to one of the
> > > CCI ports) but at worst it's just a necessary evil to make the binding
> > > happy (if it literally only represents the point-to-point link between
> > > the device master port and interconnect slave port).
> > >
> >
> > Quick update: so I adjusted to device tree according to your example and
> > it works so now I can get rid of that nasty kernel arg based workaround,
> > yey! :-)
>
> Great that we have a generic solution like I hoped for! So you will
> submit a new revision of the series to include these dts updates,
> right?
>
Yes, I already have it prepared. Just delaying the v2 for a few days maybe there will be some more feedback.
---
Best Regards, Laurentiu
^ permalink raw reply
* Re: [PATCH net-next 17/22] hv_netvsc: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 1:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: davem, dmitry.tarnyagin, wg, mkl, michal.simek, hsweeten,
madalin.bucur, pantelis.antoniou, claudiu.manoil, leoyang.li,
linux, sammy, ralf, nico, steve.glendinning, f.fainelli,
grygorii.strashko, w-kwok2, m-karicheri2, t.sailer, jreuter, kys,
haiyangz, wei.liu2, paul.durrant, arvid.brodin, pshelar, dev,
linux-mips, xen-devel, netdev, linux-usb
In-Reply-To: <20180920074341.3acef75c@xeon-e3>
On 2018/9/20 22:43, Stephen Hemminger wrote:
> On Thu, 20 Sep 2018 20:33:01 +0800
> YueHaibing <yuehaibing@huawei.com> wrote:
>
>> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
>> which is a typedef for an enum type, so make sure the implementation in
>> this driver has returns 'netdev_tx_t' value, and change the function
>> return type to netdev_tx_t.
>>
>> Found by coccinelle.
>>
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 3af6d8d..056c472 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -511,7 +511,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
>> return rc;
>> }
>>
>> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> +static netdev_tx_t
>> +netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> {
>> struct net_device_context *net_device_ctx = netdev_priv(net);
>> struct hv_netvsc_packet *packet = NULL;
>> @@ -528,8 +529,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> */
>> vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
>> if (vf_netdev && netif_running(vf_netdev) &&
>> - !netpoll_tx_running(net))
>> - return netvsc_vf_xmit(net, vf_netdev, skb);
>> + !netpoll_tx_running(net)) {
>> + ret = netvsc_vf_xmit(net, vf_netdev, skb);
>> + if (ret)
>> + return NETDEV_TX_BUSY;
>> + }
>
> Sorry, the new code is wrong. It will fall through if ret == 0 (NETDEV_TX_OK)
> Please review and test your patches.
I'm sorry for this, will correct it as Haiyang's suggestion.
>
> .
>
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-21 7:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <028b4cea-0e3f-fab5-7a74-cf003bbd1134@gmail.com>
> On Sep 20, 2018, at 4:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 04:43 PM, Song Liu wrote:
>>
>
>> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
>> the issue. However, netpoll will drop (fail to send) more packets.
>>
>
> Why is it failing ?
>
> If you are under high memory pressure, then maybe if you absolutely want memory to send
> netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
> from feeding incoming packets to the host and add more memory pressure ;)
>
I did the test with Eric's latest patch (and disable ndo_poll_controller
in driver). The result didn't show significant increase in drop packets.
I guess packet drops in my earlier test was caused by some other changes
I mixed there.
So I think this patch does fix the issue. Thanks Eric!
For ixgbe, I think we need to check napi_complete_done() return value
anyway. Otherwise, the driver will enable IRQ in polling mode.
Song
^ permalink raw reply
* RE: [PATCH net-next] net/tls: Add support for async encryption of records for performance
From: Vakul Garg @ 2018-09-21 1:14 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
davejwatson@fb.com, doronrk@fb.com
In-Reply-To: <20180920.111833.80229845984902983.davem@davemloft.net>
> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, September 20, 2018 11:49 PM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] net/tls: Add support for async encryption of
> records for performance
>
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Wed, 19 Sep 2018 20:51:35 +0530
>
> > This patch enables encryption of multiple records in parallel when an
> > async capable crypto accelerator is present in system.
>
> This seems to be trading off zero copy with async support.
>
> Async crypto device support is not the common case at all, and synchronous
> crypto via cpu crypto acceleration instructions is so much more likely.
>
> Oh I see, the new logic is only triggered with ASYNC_CAPABLE is set?
>
> > +static inline bool is_tx_ready(struct tls_context *tls_ctx,
> > + struct tls_sw_context_tx *ctx)
> > +{
>
> Two space between "inline" and "bool", please make it one.
Fixed.
Seems checkpatch misses it.
>
> > static void tls_write_space(struct sock *sk) {
> > struct tls_context *ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>
> Longest to shortest line (reverse christmas tree) ordering for local variable
> declarations please.
Can't do this. The second variable assignment is dependent upon previous one.
> >
> > + list_for_each_prev(pos, &ctx->tx_ready_list) {
> > + struct tls_rec *rec = (struct tls_rec *)pos;
> > + u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
>
> Likewise.
>
I can split variable declaration 'seq' and its assignment into two separate lines.
But I am not sure if increasing number of lines in order to comply reverse Christmas tree
is a good thing for this case.
> > -static int tls_do_encryption(struct tls_context *tls_ctx,
> > +int tls_tx_records(struct sock *sk, int flags) {
> > + struct tls_rec *rec, *tmp;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + int rc = 0;
> > + int tx_flags;
>
> Likewise.
Could partially address since ctx assignment depends upon tls_ctx assignment.
>
> > +static void tls_encrypt_done(struct crypto_async_request *req, int
> > +err) {
> > + struct aead_request *aead_req = (struct aead_request *)req;
> > + struct sock *sk = req->data;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
> > + int pending;
> > + bool ready = false;
>
> Likewise.
Placed 'ready' above pending 'pending'. Rest unchanged because of dependencies.
>
> > +static int tls_do_encryption(struct sock *sk,
> > + struct tls_context *tls_ctx,
> > struct tls_sw_context_tx *ctx,
> > struct aead_request *aead_req,
> > size_t data_len)
> > {
> > int rc;
> > + struct tls_rec *rec = ctx->open_rec;
>
> Likewise.
>
> > @@ -473,11 +630,12 @@ static int memcopy_from_iter(struct sock *sk,
> > struct iov_iter *from, {
> > struct tls_context *tls_ctx = tls_get_ctx(sk);
> > struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > - struct scatterlist *sg = ctx->sg_plaintext_data;
> > + struct tls_rec *rec = ctx->open_rec;
> > + struct scatterlist *sg = rec->sg_plaintext_data;
> > int copy, i, rc = 0;
>
> Likewise.
Can't change because of dependencies.
>
> > +struct tls_rec *get_rec(struct sock *sk) {
> > + int mem_size;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
>
> Likewise.
>
Declared 'mem_size' below 'rec'.
> > @@ -510,21 +707,33 @@ int tls_sw_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > int record_room;
> > bool full_record;
> > int orig_size;
> > + struct tls_rec *rec;
> > bool is_kvec = msg->msg_iter.type & ITER_KVEC;
> > + struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
> > + bool async_capable = tfm->__crt_alg->cra_flags &
> CRYPTO_ALG_ASYNC;
> > + int num_async = 0;
> > + int num_zc = 0;
>
> Likewise.
Fixed
> > @@ -661,6 +904,8 @@ int tls_sw_sendpage(struct sock *sk, struct page
> *page,
> > struct scatterlist *sg;
> > bool full_record;
> > int record_room;
> > + struct tls_rec *rec;
> > + int num_async = 0;
>
> Likewise.
Fixed.
Sending v2.
^ permalink raw reply
* Re: [PATCH iproute2-next] iplink: add ipvtap support
From: David Ahern @ 2018-09-21 0:55 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Stephen Hemminger, Phil Sutter, Sainath Grandhi, Davide Caratti
In-Reply-To: <1537326209-30837-1-git-send-email-liuhangbin@gmail.com>
On 9/18/18 8:03 PM, Hangbin Liu wrote:
> IPVLAN and IPVTAP are using the same functions and parameters. So we can
> just add a new link_util with id ipvtap. Others are the same.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> ip/iplink.c | 4 ++--
> ip/iplink_ipvlan.c | 28 ++++++++++++++++++----------
> man/man8/ip-link.8.in | 4 ++++
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
applied to iproute2-next. Thanks
^ permalink raw reply
* [PATCH net] net/ipv6: Display all addresses in output of /proc/net/if_inet6
From: Jeff Barnhill @ 2018-09-21 0:45 UTC (permalink / raw)
To: netdev; +Cc: davem, kuznet, yoshfuji, Jeff Barnhill
The backend handling for /proc/net/if_inet6 in addrconf.c doesn't properly
handle starting/stopping the iteration. The problem is that at some point
during the iteration, an overflow is detected and the process is
subsequently stopped. The item being shown via seq_printf() when the
overflow occurs is not actually shown, though. When start() is
subsequently called to resume iterating, it returns the next item, and
thus the item that was being processed when the overflow occurred never
gets printed.
Alter the meaning of the private data member "offset". Currently, when it
is not 0 (which only happens at the very beginning), "offset" represents
the next hlist item to be printed. After this change, "offset" always
represents the current item.
This is also consistent with the private data member "bucket", which
represents the current bucket, and also the use of "pos" as defined in
seq_file.txt:
The pos passed to start() will always be either zero, or the most
recent pos used in the previous session.
Signed-off-by: Jeff Barnhill <0xeffeff@gmail.com>
---
net/ipv6/addrconf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d51a8c0b3372..c63ccce6425f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4201,7 +4201,6 @@ static struct inet6_ifaddr *if6_get_first(struct seq_file *seq, loff_t pos)
p++;
continue;
}
- state->offset++;
return ifa;
}
@@ -4225,13 +4224,12 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
return ifa;
}
+ state->offset = 0;
while (++state->bucket < IN6_ADDR_HSIZE) {
- state->offset = 0;
hlist_for_each_entry_rcu(ifa,
&inet6_addr_lst[state->bucket], addr_lst) {
if (!net_eq(dev_net(ifa->idev->dev), net))
continue;
- state->offset++;
return ifa;
}
}
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the vSwitch
From: Stephen Hemminger @ 2018-09-21 0:41 UTC (permalink / raw)
To: Haiyang Zhang
Cc: davem@davemloft.net, netdev@vger.kernel.org, olaf@aepfle.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
vkuznets
In-Reply-To: <BN6PR21MB01614DCE1099980A7B1286FFCA130@BN6PR21MB0161.namprd21.prod.outlook.com>
On Thu, 20 Sep 2018 20:56:46 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, September 20, 2018 4:48 PM
> > To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> > netdev@vger.kernel.org; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; vkuznets <vkuznets@redhat.com>
> > Subject: Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the
> > vSwitch
> >
> > On Thu, 20 Sep 2018 17:06:59 +0000
> > Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> >
> > > +static inline void rsc_add_data
> > > + (struct netvsc_channel *nvchan,
> > > + const struct ndis_pkt_8021q_info *vlan,
> > > + const struct ndis_tcp_ip_checksum_info *csum_info,
> > > + void *data, u32 len)
> > > +{
> >
> > Could this be changed to look more like a function and skip the inline.
> > The compiler will end up inlining it anyway.
> >
> > static void rsc_add_data(struct netvsc_channel *nvchan,
>
> How about this?
> static inline
> void rsc_add_data(struct netvsc_channel *nvchan,
>
Sure that matches other code in that file
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in _decode_session6
From: Alexei Starovoitov @ 2018-09-21 6:21 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Eric Dumazet, syzbot, Alexei Starovoitov, Daniel Borkmann,
David Miller, Herbert Xu, Alexey Kuznetsov, LKML, netdev,
Steffen Klassert, syzkaller-bugs, Hideaki YOSHIFUJI
On Thu, Sep 6, 2018 at 12:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> but I have a hard time reproducing the issue, so will appreciate
>> if somebody can test the following patch:
>
> syzbot can:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
was the patch tested?
it seems to me syzbot doesn't care about kernel quality but rather
about the number of issues syzbot can find.
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Eric Dumazet @ 2018-09-20 23:49 UTC (permalink / raw)
To: Song Liu, Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <E4A7BD0D-B091-4FBD-94A9-F0104729DCF6@fb.com>
On 09/20/2018 04:43 PM, Song Liu wrote:
>
> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
> the issue. However, netpoll will drop (fail to send) more packets.
>
Why is it failing ?
If you are under high memory pressure, then maybe if you absolutely want memory to send
netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
from feeding incoming packets to the host and add more memory pressure ;)
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-20 23:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <ee9b9a55-c967-cec3-7df1-79f84b06154c@gmail.com>
> On Sep 20, 2018, at 4:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 03:42 PM, Song Liu wrote:
>>
>>
>>> On Sep 20, 2018, at 2:01 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>>>
>>> On Thu, 2018-09-20 at 13:35 -0700, Eric Dumazet wrote:
>>>> On 09/20/2018 12:01 PM, Song Liu wrote:
>>>>> The NIC driver should only enable interrupts when napi_complete_done()
>>>>> returns true. This patch adds the check for ixgbe.
>>>>>
>>>>> Cc: stable@vger.kernel.org # 4.10+
>>>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>>>> Suggested-by: Eric Dumazet <edumazet@google.com>
>>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>>> ---
>>>>
>>>>
>>>> Well, unfortunately we do not know why this is needed,
>>>> this is why I have not yet sent this patch formally.
>>>>
>>>> netpoll has correct synchronization :
>>>>
>>>> poll_napi() places into napi->poll_owner current cpu number before
>>>> calling poll_one_napi()
>>>>
>>>> netpoll_poll_lock() does also use napi->poll_owner
>>>>
>>>> When netpoll calls ixgbe poll() method, it passed a budget of 0,
>>>> meaning napi_complete_done() is not called.
>>>>
>>>> As long as we can not explain the problem properly in the changelog,
>>>> we should investigate, otherwise we will probably see coming dozens of
>>>> patches
>>>> trying to fix a 'potential hazard'.
>>>
>>> Agreed, which is why I have our validation and developers looking into it,
>>> while we test the current patch from Song.
>>
>> I figured out what is the issue here. And I have a proposal to fix it. I
>> have verified that this fixes the issue in our tests. But Alexei suggests
>> that it may not be the right way to fix.
>>
>> Here is what happened:
>>
>> netpoll tries to send skb with netpoll_start_xmit(). If that fails, it
>> calls netpoll_poll_dev(), which calls ndo_poll_controller(). Then, in
>> the driver, ndo_poll_controller() calls napi_schedule() for ALL NAPIs
>> within the same NIC.
>>
>> This is problematic, because at the end napi_schedule() calls:
>>
>> ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>
>> which attached these NAPIs to softnet_data on THIS CPU. This is done
>> via napi->poll_list.
>>
>> Then suddenly ksoftirqd on this CPU owns multiple NAPIs. And it will
>> not give up the ownership until it calls napi_complete_done(). However,
>> for a very busy server, we usually use 16 CPUs to poll NAPI, so this
>> CPU can easily be overloaded. And as a result, each call of napi->poll()
>> will hit budget (of 64), and it will not call napi_complete_done(),
>> and the NAPI stays in the poll_list of this CPU.
>>
>> When this happens, the host usually cannot get out of this state until
>> we throttle/stop client traffic.
>>
>>
>> I am pretty confident this is what happened. Please let me know if
>> anything above doesn't make sense.
>>
>>
>> Here is my proposal to fix it: Instead of polling all NAPIs within one
>> NIC, I would have netpoll to only poll the NAPI that will free space
>> for netpoll_start_xmit(). I attached my two RFC patches to the end of
>> this email.
>>
>> I chatted with Alexei about this. He think polling only one NAPI may
>> not guarantee netpoll make progress with the TX queue we are aiming
>> for. Also, the bigger problem may be the fact that NAPIs could get
>> pinned to one CPU and cannot get released.
>>
>> At this point, I really don't know what is the best way to fix this.
>>
>> I will also work on a repro with netperf.
>
> Thanks !
>
>>
>> Please let me know your suggestions.
>>
>
> Yeah, maybe that NICs using NAPI could not provide an ndo_poll_controller() method at all,
> since it is very risky (potentially grab many NAPI, and end up in this locked situation)
>
> poll_napi() could attempt to free skbs one napi at a time,
> without the current cpu stealing all NAPI.
>
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 57557a6a950cc9cdff959391576a03381d328c1a..a992971d366090ba69d5c1af32eadd554d6880cf 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -205,13 +205,8 @@ static void netpoll_poll_dev(struct net_device *dev)
> }
>
> ops = dev->netdev_ops;
> - if (!ops->ndo_poll_controller) {
> - up(&ni->dev_lock);
> - return;
> - }
> -
> - /* Process pending work on NIC */
> - ops->ndo_poll_controller(dev);
> + if (ops->ndo_poll_controller)
> + ops->ndo_poll_controller(dev);
>
> poll_napi(dev);
>
I tried to totally skip ndo_poll_controller() here. It did avoid hitting
the issue. However, netpoll will drop (fail to send) more packets.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] net: netronome: remove redundant continue
From: zhong jiang @ 2018-09-21 5:21 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, dirk.vandermerwe, simon.horman, netdev, linux-kernel
In-Reply-To: <20180920093857.6227710f@cakuba.netronome.com>
On 2018/9/21 0:38, Jakub Kicinski wrote:
> On Thu, 20 Sep 2018 16:02:21 +0800, zhong jiang wrote:
>> The continue will not truely skip any code. hence it is safe to
>> remove it.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> I think this came up during review at some point. I still prefer to
> keep the continue. The body of the loop performs initialization of
> objects, if an object is removed we shouldn't carry on with the body.
> It's easy to miss that the object got freed otherwise, there is no
> error being set and no warning printed...
IMO, we should bring it back when the case truely occur you have said. At present.
We should not take too much into account. Maybe it will not occur.
Thanks,
zhong jiang
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> index 0b1ac9c..50d7b58 100644
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> @@ -230,10 +230,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
>> ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>>
>> /* Kill the vNIC if app init marked it as invalid */
>> - if (nn->port && nn->port->type == NFP_PORT_INVALID) {
>> + if (nn->port && nn->port->type == NFP_PORT_INVALID)
>> nfp_net_pf_free_vnic(pf, nn);
>> - continue;
>> - }
>> }
>>
>> if (list_empty(&pf->vnics))
>
> .
>
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Andy Lutomirski @ 2018-09-21 4:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A. Donenfeld, Andrew Lunn, Arnd Bergmann, Eric Biggers,
LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAKv+Gu-sciHeWVij8hWGF78HV_CLddMv7Xdf1GRSY+6B2yt48A@mail.gmail.com>
> On Sep 20, 2018, at 9:30 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 20 September 2018 at 21:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Hi Andy,
>>
>>> On Fri, Sep 21, 2018 at 5:23 AM Andy Lutomirski <luto@amacapital.net> wrote:
>>> At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
>>>
>>> kernel_fpu_call(func, arg);
>>>
>>> And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
>>>
>>> I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
>>
>> Haha. That's fun, and maybe we'll do that at some point, but I have
>> some other reasons too for being on a workqueue now.
>>
>
> Kernel mode crypto is callable from any context, and SIMD can be used
> in softirq context on arm64 (and on x86, even from hardirq context
> IIRC if the interrupt is taken from userland), in which case we'd
> already be on the irq stack.
The x86_64 irq stack handles nesting already.
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: David Ahern @ 2018-09-20 22:53 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev@vger.kernel.org
In-Reply-To: <20180920140617.uybaha3n7iz6ckvn@breakpoint.cc>
On 9/20/18 7:06 AM, Florian Westphal wrote:
> David Ahern <dsahern@gmail.com> wrote:
>>> $ make O=kbuild/perf -j 24 -s
>>> In file included from /home/dsa/kernel-3.git/include/linux/kernel.h:10:0,
>>> from /home/dsa/kernel-3.git/include/linux/list.h:9,
>>> from /home/dsa/kernel-3.git/include/linux/module.h:9,
>>> from /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c:13:
>>> /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c: In function
>>> ‘xfrm_output_resume’:
>>> /home/dsa/kernel-3.git/include/linux/compiler.h:252:20: warning: array
>>> subscript is above array bounds [-Warray-bounds]
>>> __read_once_size(&(x), __u.__c, sizeof(x)); \
>
> Does this thing avoid the warning?
nope, still see it.
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-21 4:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Lunn, Arnd Bergmann, Ard Biesheuvel, Eric Biggers, LKML,
Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <8FA361E3-FBCB-469E-88DC-F4085BD91175@amacapital.net>
Hi Andy,
On Fri, Sep 21, 2018 at 5:23 AM Andy Lutomirski <luto@amacapital.net> wrote:
> At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
>
> kernel_fpu_call(func, arg);
>
> And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
>
> I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
Haha. That's fun, and maybe we'll do that at some point, but I have
some other reasons too for being on a workqueue now.
>
> All that being said, why are these frames so large? It sounds like something may be spilling that ought not to.
They're not. Well, they're not anymore. I had a silly thing before
like "u8 buffer[1 << 12]" in some debugging code, which is what
prompted the ccflag-y addition. I cleaned up the mistakes like that
and frames are now reasonable everywhere. Non-issue.
Jason
^ permalink raw reply
* Re: [PATCH net] af_key: free SKBs under RCU protection
From: Eric Dumazet @ 2018-09-20 22:29 UTC (permalink / raw)
To: Eric Dumazet, stranche; +Cc: netdev, steffen.klassert
In-Reply-To: <82b2ac27-0f61-3be6-b09e-ec31cf95881f@gmail.com>
On 09/20/2018 03:10 PM, Eric Dumazet wrote:
>
>
> On 09/20/2018 12:25 PM, stranche@codeaurora.org wrote:
>>>
>>> I do not believe the changelog or the patch makes sense.
>>>
>>> Having skb still referencing a socket prevents this socket being released.
>>>
>>> If you think about it, what would prevent the freeing happening
>>> _before_ the rcu_read_lock() in pfkey_broadcast() ?
>>>
>>> Maybe the correct fix is that pfkey_broadcast_one() should ensure the
>>> socket is still valid.
>>>
>>> I would suggest something like :
>>>
>>> diff --git a/net/key/af_key.c b/net/key/af_key.c
>>> index
>>> 9d61266526e767770d9a1ce184ac8cdd59de309a..5ce309d020dda5e46e4426c4a639bfb551e2260d
>>> 100644
>>> --- a/net/key/af_key.c
>>> +++ b/net/key/af_key.c
>>> @@ -201,7 +201,9 @@ static int pfkey_broadcast_one(struct sk_buff
>>> *skb, struct sk_buff **skb2,
>>> {
>>> int err = -ENOBUFS;
>>>
>>> - sock_hold(sk);
>>> + if (!refcount_inc_not_zero(&sk->sk_refcnt))
>>> + return -ENOENT;
>>> +
>>> if (*skb2 == NULL) {
>>> if (refcount_read(&skb->users) != 1) {
>>> *skb2 = skb_clone(skb, allocation);
>>
>> Hi Eric,
>>
>> I'm not sure that the socket getting freed before the rcu_read_lock() would
>> be an issue, since then it would no longer be in the net_pkey->table that
>> we loop through (since we call pfkey_remove() from pfkey_relase()). Because of
>> that, all the sockets processed in pfkey_broadcast_one() have valid refcounts,
>> so checking for zero there doesn't prevent the crash that I'm seeing.
>>
>> However, after going over the call flow again, I see that the actual problem
>> occurs because of pfkey_broadcast_one(). Specifically, because of this check:
>>
>> if (*skb2 == NULL) {
>> if (refcount_read(&skb->users) != 1) {
>> *skb2 = skb_clone(skb, allocation);
>> } else {
>> *skb2 = skb;
>> refcount_inc(&skb->users);
>> }
>> }
>>
>> Since we always pass a freshly cloned SKB to this function, skb->users is
>> always 1, and skb2 just becomes skb. We then set skb2 (and thus skb) to
>> belong to the socket.
>>
>> If the socket we queue skb2 to frees this SKB (thereby decrementing its
>> refcount to 1) and the socket is freed before pfkey_broadcast() can
>> execute the kfree_skb(skb) on line 284, we will then attempt to run
>> sock_rfree() on an SKB with a dangling reference to this socket.
>>
>> Perhaps a cleaner solution here is to always clone the SKB in
>> pfkey_broadcast_one(). That will ensure that the two kfree_skb() calls
>> in pfkey_broadcast() will never be passed an SKB with sock_rfree() as
>> its destructor, and we can avoid this race condition.
>
> As long as one skb has sock_rfree has its destructor, the socket attached to
> this skb can not be released. There is no race here.
>
> Note that skb_clone() does not propagate the destructor.
>
> The issue here is that in the rcu lookup, we can find a socket that has been
> dismantled, with a 0 refcount.
>
> We must not use sock_hold() in this case, since we are not sure the socket refcount is not already 0.
>
> pfkey_broadcast() and pfkey_broadcast_one() violate basic RCU rules.
>
> When in a RCU lookup, one want to increment an object refcount, it needs
> to be extra-careful, as I did in my proposal.
>
> Note that the race could be automatically detected with CONFIG_REFCOUNT_FULL=y
Bug was added in commit 7f6b9dbd5afb ("af_key: locking change")
^ permalink raw reply
* Re: [PATCH net] af_key: free SKBs under RCU protection
From: Eric Dumazet @ 2018-09-20 22:10 UTC (permalink / raw)
To: stranche, Eric Dumazet; +Cc: netdev, steffen.klassert
In-Reply-To: <357e28c3fa0c7bacaffde4e960f58a87@codeaurora.org>
On 09/20/2018 12:25 PM, stranche@codeaurora.org wrote:
>>
>> I do not believe the changelog or the patch makes sense.
>>
>> Having skb still referencing a socket prevents this socket being released.
>>
>> If you think about it, what would prevent the freeing happening
>> _before_ the rcu_read_lock() in pfkey_broadcast() ?
>>
>> Maybe the correct fix is that pfkey_broadcast_one() should ensure the
>> socket is still valid.
>>
>> I would suggest something like :
>>
>> diff --git a/net/key/af_key.c b/net/key/af_key.c
>> index
>> 9d61266526e767770d9a1ce184ac8cdd59de309a..5ce309d020dda5e46e4426c4a639bfb551e2260d
>> 100644
>> --- a/net/key/af_key.c
>> +++ b/net/key/af_key.c
>> @@ -201,7 +201,9 @@ static int pfkey_broadcast_one(struct sk_buff
>> *skb, struct sk_buff **skb2,
>> {
>> int err = -ENOBUFS;
>>
>> - sock_hold(sk);
>> + if (!refcount_inc_not_zero(&sk->sk_refcnt))
>> + return -ENOENT;
>> +
>> if (*skb2 == NULL) {
>> if (refcount_read(&skb->users) != 1) {
>> *skb2 = skb_clone(skb, allocation);
>
> Hi Eric,
>
> I'm not sure that the socket getting freed before the rcu_read_lock() would
> be an issue, since then it would no longer be in the net_pkey->table that
> we loop through (since we call pfkey_remove() from pfkey_relase()). Because of
> that, all the sockets processed in pfkey_broadcast_one() have valid refcounts,
> so checking for zero there doesn't prevent the crash that I'm seeing.
>
> However, after going over the call flow again, I see that the actual problem
> occurs because of pfkey_broadcast_one(). Specifically, because of this check:
>
> if (*skb2 == NULL) {
> if (refcount_read(&skb->users) != 1) {
> *skb2 = skb_clone(skb, allocation);
> } else {
> *skb2 = skb;
> refcount_inc(&skb->users);
> }
> }
>
> Since we always pass a freshly cloned SKB to this function, skb->users is
> always 1, and skb2 just becomes skb. We then set skb2 (and thus skb) to
> belong to the socket.
>
> If the socket we queue skb2 to frees this SKB (thereby decrementing its
> refcount to 1) and the socket is freed before pfkey_broadcast() can
> execute the kfree_skb(skb) on line 284, we will then attempt to run
> sock_rfree() on an SKB with a dangling reference to this socket.
>
> Perhaps a cleaner solution here is to always clone the SKB in
> pfkey_broadcast_one(). That will ensure that the two kfree_skb() calls
> in pfkey_broadcast() will never be passed an SKB with sock_rfree() as
> its destructor, and we can avoid this race condition.
As long as one skb has sock_rfree has its destructor, the socket attached to
this skb can not be released. There is no race here.
Note that skb_clone() does not propagate the destructor.
The issue here is that in the rcu lookup, we can find a socket that has been
dismantled, with a 0 refcount.
We must not use sock_hold() in this case, since we are not sure the socket refcount is not already 0.
pfkey_broadcast() and pfkey_broadcast_one() violate basic RCU rules.
When in a RCU lookup, one want to increment an object refcount, it needs
to be extra-careful, as I did in my proposal.
Note that the race could be automatically detected with CONFIG_REFCOUNT_FULL=y
^ permalink raw reply
* [PATCH net-next] net: apple: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 3:46 UTC (permalink / raw)
To: davem, keescook; +Cc: linux-kernel, netdev, fthain, YueHaibing
The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
which is a typedef for an enum type, so make sure the implementation in
this driver has returns 'netdev_tx_t' value, and change the function
return type to netdev_tx_t.
Found by coccinelle.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/apple/bmac.c | 4 ++--
drivers/net/ethernet/apple/mace.c | 4 ++--
drivers/net/ethernet/apple/macmace.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/apple/bmac.c b/drivers/net/ethernet/apple/bmac.c
index 024998d..6a8e256 100644
--- a/drivers/net/ethernet/apple/bmac.c
+++ b/drivers/net/ethernet/apple/bmac.c
@@ -154,7 +154,7 @@ struct bmac_data {
static irqreturn_t bmac_rxdma_intr(int irq, void *dev_id);
static void bmac_set_timeout(struct net_device *dev);
static void bmac_tx_timeout(struct timer_list *t);
-static int bmac_output(struct sk_buff *skb, struct net_device *dev);
+static netdev_tx_t bmac_output(struct sk_buff *skb, struct net_device *dev);
static void bmac_start(struct net_device *dev);
#define DBDMA_SET(x) ( ((x) | (x) << 16) )
@@ -1456,7 +1456,7 @@ static int bmac_close(struct net_device *dev)
spin_unlock_irqrestore(&bp->lock, flags);
}
-static int
+static netdev_tx_t
bmac_output(struct sk_buff *skb, struct net_device *dev)
{
struct bmac_data *bp = netdev_priv(dev);
diff --git a/drivers/net/ethernet/apple/mace.c b/drivers/net/ethernet/apple/mace.c
index 0b5429d..68b9ee4 100644
--- a/drivers/net/ethernet/apple/mace.c
+++ b/drivers/net/ethernet/apple/mace.c
@@ -78,7 +78,7 @@ struct mace_data {
static int mace_open(struct net_device *dev);
static int mace_close(struct net_device *dev);
-static int mace_xmit_start(struct sk_buff *skb, struct net_device *dev);
+static netdev_tx_t mace_xmit_start(struct sk_buff *skb, struct net_device *dev);
static void mace_set_multicast(struct net_device *dev);
static void mace_reset(struct net_device *dev);
static int mace_set_address(struct net_device *dev, void *addr);
@@ -525,7 +525,7 @@ static inline void mace_set_timeout(struct net_device *dev)
mp->timeout_active = 1;
}
-static int mace_xmit_start(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t mace_xmit_start(struct sk_buff *skb, struct net_device *dev)
{
struct mace_data *mp = netdev_priv(dev);
volatile struct dbdma_regs __iomem *td = mp->tx_dma;
diff --git a/drivers/net/ethernet/apple/macmace.c b/drivers/net/ethernet/apple/macmace.c
index 137cbb4..376f2c2 100644
--- a/drivers/net/ethernet/apple/macmace.c
+++ b/drivers/net/ethernet/apple/macmace.c
@@ -89,7 +89,7 @@ struct mace_frame {
static int mace_open(struct net_device *dev);
static int mace_close(struct net_device *dev);
-static int mace_xmit_start(struct sk_buff *skb, struct net_device *dev);
+static netdev_tx_t mace_xmit_start(struct sk_buff *skb, struct net_device *dev);
static void mace_set_multicast(struct net_device *dev);
static int mace_set_address(struct net_device *dev, void *addr);
static void mace_reset(struct net_device *dev);
@@ -444,7 +444,7 @@ static int mace_close(struct net_device *dev)
* Transmit a frame
*/
-static int mace_xmit_start(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t mace_xmit_start(struct sk_buff *skb, struct net_device *dev)
{
struct mace_data *mp = netdev_priv(dev);
unsigned long flags;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next] net: i825xx: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 3:44 UTC (permalink / raw)
To: davem, linux, sammy; +Cc: linux-kernel, netdev, linux-arm-kernel, YueHaibing
The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
which is a typedef for an enum type, so make sure the implementation in
this driver has returns 'netdev_tx_t' value, and change the function
return type to netdev_tx_t.
Found by coccinelle.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/i825xx/ether1.c | 5 +++--
drivers/net/ethernet/i825xx/lib82596.c | 4 ++--
drivers/net/ethernet/i825xx/sun3_82586.c | 6 ++++--
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/i825xx/ether1.c b/drivers/net/ethernet/i825xx/ether1.c
index dc98345..35f6291 100644
--- a/drivers/net/ethernet/i825xx/ether1.c
+++ b/drivers/net/ethernet/i825xx/ether1.c
@@ -64,7 +64,8 @@
#define RX_AREA_END 0x0fc00
static int ether1_open(struct net_device *dev);
-static int ether1_sendpacket(struct sk_buff *skb, struct net_device *dev);
+static netdev_tx_t ether1_sendpacket(struct sk_buff *skb,
+ struct net_device *dev);
static irqreturn_t ether1_interrupt(int irq, void *dev_id);
static int ether1_close(struct net_device *dev);
static void ether1_setmulticastlist(struct net_device *dev);
@@ -667,7 +668,7 @@
netif_wake_queue(dev);
}
-static int
+static netdev_tx_t
ether1_sendpacket (struct sk_buff *skb, struct net_device *dev)
{
int tmp, tst, nopaddr, txaddr, tbdaddr, dataddr;
diff --git a/drivers/net/ethernet/i825xx/lib82596.c b/drivers/net/ethernet/i825xx/lib82596.c
index f00a1dc..2f7ae11 100644
--- a/drivers/net/ethernet/i825xx/lib82596.c
+++ b/drivers/net/ethernet/i825xx/lib82596.c
@@ -347,7 +347,7 @@ struct i596_private {
0x7f /* *multi IA */ };
static int i596_open(struct net_device *dev);
-static int i596_start_xmit(struct sk_buff *skb, struct net_device *dev);
+static netdev_tx_t i596_start_xmit(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t i596_interrupt(int irq, void *dev_id);
static int i596_close(struct net_device *dev);
static void i596_add_cmd(struct net_device *dev, struct i596_cmd *cmd);
@@ -966,7 +966,7 @@ static void i596_tx_timeout (struct net_device *dev)
}
-static int i596_start_xmit(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t i596_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct i596_private *lp = netdev_priv(dev);
struct tx_cmd *tx_cmd;
diff --git a/drivers/net/ethernet/i825xx/sun3_82586.c b/drivers/net/ethernet/i825xx/sun3_82586.c
index 8bb15a8..1a86184 100644
--- a/drivers/net/ethernet/i825xx/sun3_82586.c
+++ b/drivers/net/ethernet/i825xx/sun3_82586.c
@@ -121,7 +121,8 @@
static irqreturn_t sun3_82586_interrupt(int irq,void *dev_id);
static int sun3_82586_open(struct net_device *dev);
static int sun3_82586_close(struct net_device *dev);
-static int sun3_82586_send_packet(struct sk_buff *,struct net_device *);
+static netdev_tx_t sun3_82586_send_packet(struct sk_buff *,
+ struct net_device *);
static struct net_device_stats *sun3_82586_get_stats(struct net_device *dev);
static void set_multicast_list(struct net_device *dev);
static void sun3_82586_timeout(struct net_device *dev);
@@ -1002,7 +1003,8 @@ static void sun3_82586_timeout(struct net_device *dev)
* send frame
*/
-static int sun3_82586_send_packet(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t
+sun3_82586_send_packet(struct sk_buff *skb, struct net_device *dev)
{
int len,i;
#ifndef NO_NOPCOMMANDS
--
1.8.3.1
^ permalink raw reply related
* [PATCH nf-next] netfilter: ctnetlink: must check mark attributes vs NULL
From: Florian Westphal @ 2018-09-20 21:53 UTC (permalink / raw)
To: netfilter-devel
Cc: syzbot+e45eda8eda6e93a03959, syzkaller-bugs, netdev,
kristian.evensen, Florian Westphal
In-Reply-To: <000000000000f4bf02057653dc0a@google.com>
else we will oops (null deref) when the attributes aren't present.
Also add back the EOPNOTSUPP in case MARK filtering is requested but
kernel doesn't support it.
Fixes: 59c08c69c2788 ("netfilter: ctnetlink: Support L3 protocol-filter on flush")
Reported-by: syzbot+e45eda8eda6e93a03959@syzkaller.appspotmail.com
Signed-off-by: Florian Westphal <fw@strlen.de>
---
only affects nf-next and linux-next.
net/netfilter/nf_conntrack_netlink.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index f8c74f31aa36..b7a6984c74d0 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -833,6 +833,11 @@ ctnetlink_alloc_filter(const struct nlattr * const cda[], u8 family)
{
struct ctnetlink_filter *filter;
+#ifndef CONFIG_NF_CONNTRACK_MARK
+ if (cda[CTA_MARK] && cda[CTA_MARK_MASK])
+ return ERR_PTR(-EOPNOTSUPP);
+#endif
+
filter = kzalloc(sizeof(*filter), GFP_KERNEL);
if (filter == NULL)
return ERR_PTR(-ENOMEM);
@@ -840,8 +845,10 @@ ctnetlink_alloc_filter(const struct nlattr * const cda[], u8 family)
filter->family = family;
#ifdef CONFIG_NF_CONNTRACK_MARK
- filter->mark.val = ntohl(nla_get_be32(cda[CTA_MARK]));
- filter->mark.mask = ntohl(nla_get_be32(cda[CTA_MARK_MASK]));
+ if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) {
+ filter->mark.val = ntohl(nla_get_be32(cda[CTA_MARK]));
+ filter->mark.mask = ntohl(nla_get_be32(cda[CTA_MARK_MASK]));
+ }
#endif
return filter;
}
--
2.16.4
^ permalink raw reply related
* [PATCH net-next] net: wiznet: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 3:35 UTC (permalink / raw)
To: davem; +Cc: linux-kernel, netdev, YueHaibing
The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
which is a typedef for an enum type, so make sure the implementation in
this driver has returns 'netdev_tx_t' value, and change the function
return type to netdev_tx_t.
Found by coccinelle.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/wiznet/w5100.c | 2 +-
drivers/net/ethernet/wiznet/w5300.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
index 2bdfb39..d8ba512 100644
--- a/drivers/net/ethernet/wiznet/w5100.c
+++ b/drivers/net/ethernet/wiznet/w5100.c
@@ -835,7 +835,7 @@ static void w5100_tx_work(struct work_struct *work)
w5100_tx_skb(priv->ndev, skb);
}
-static int w5100_start_tx(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t w5100_start_tx(struct sk_buff *skb, struct net_device *ndev)
{
struct w5100_priv *priv = netdev_priv(ndev);
diff --git a/drivers/net/ethernet/wiznet/w5300.c b/drivers/net/ethernet/wiznet/w5300.c
index 56ae573..80fdbff 100644
--- a/drivers/net/ethernet/wiznet/w5300.c
+++ b/drivers/net/ethernet/wiznet/w5300.c
@@ -365,7 +365,7 @@ static void w5300_tx_timeout(struct net_device *ndev)
netif_wake_queue(ndev);
}
-static int w5300_start_tx(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t w5300_start_tx(struct sk_buff *skb, struct net_device *ndev)
{
struct w5300_priv *priv = netdev_priv(ndev);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next 0/7] add support for VSC8584 and VSC8574 Microsemi quad-port PHYs
From: Linus Walleij @ 2018-09-20 21:38 UTC (permalink / raw)
To: quentin.schulz
Cc: Alexandre Belloni, Ralf Baechle, paul.burton, James Hogan,
Rob Herring, Mark Rutland, David S. Miller, Andrew Lunn,
Florian Fainelli, allan.nielsen, Linux MIPS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, netdev, Thomas Petazzoni,
Antoine Tenart
In-Reply-To: <cover.b921b010b6d6bde1c11e69551ae38f3b2818645b.1536916714.git-series.quentin.schulz@bootlin.com>
Just as a drive-by comment this seems vaguely related to the Vitesse
DSA switch I merged in drivers/net/dsa/vitesse-vsc73xx.c
The VSC* product name handily gives away the origin in Vitesse's
product line.
The VSC73xx also have the 8051 CPU and internal RAM, but are
accessed (typically) over SPI, and AFAICT this thing is talking over
MDIO.
The Vitesse 73xx however also supports a WAN port and VLANs
which makes it significantly different, falling into switch class I
guess.
These VSC85*4's does have an SPI interface as well, according
to the data sheet but I assume your target boards don't even
connect it?
When it comes to 8051 code we have quite a lot of this in the kernel
these days, I suspect the 8051 snippets in this code could be
disassembled and put into linux-firmware in source form, but
that is maybe a bit overly ambitious. We have done that for a few
USB to serial controllers using the EzUSB 8051 though:
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/keyspan_pda
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/usbdux
These can rebuild their firmware using the as31 assembler.
https://github.com/nitsky/as31
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Andy Lutomirski @ 2018-09-21 3:23 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jason A. Donenfeld, Arnd Bergmann, Ard Biesheuvel, Eric Biggers,
LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <20180921031255.GB11109@lunn.ch>
> On Sep 20, 2018, at 8:12 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>
>> On Fri, Sep 21, 2018 at 02:11:43AM +0200, Jason A. Donenfeld wrote:
>> Hey Arnd,
>>
>>> On Thu, Sep 20, 2018 at 6:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>> Right, if you hit a stack requirement like this, it's usually the compiler
>>> doing something bad, not just using too much stack but also generating
>>> rather slow object code in the process. It's better to fix the bug by
>>> optimizing the code to not spill registers to the stack.
>>>
>>> In the long run, I'd like to reduce the stack frame size further, so
>>> best assume that anything over 1024 bytes (on 32-bit) or 1280 bytes
>>> (on 64-bit) is a bug in the code, and stay below that.
>>>
>>> For prototyping, you can just mark the broken functions individually
>>> by setting the warning limit for a specific function that is known to
>>> be misoptimized by the compiler (with a comment about which compiler
>>> and architectures are affected), but not override the limit for the
>>> entire file.
>>
>> Thanks for the explanation. Fortunately in my case, the issues were
>> trivially fixable to get it under 1024/1280. (By the way, why does
>> 64-bit have a slightly larger stack frame? To account for 32 pointers
>> taking double the space or something?) That will be rectified in v6.
>
> Hi Jason
>
> Do you any stack usage information?
>
> A VPN can be at the bottom of some deep stack calls. Swap on NFS over
> the VPN? If you have one frame of 1K, you might be O.K. But if you
> have a few of these, i can see there might be issues of overflowing
> the stack.
>
>
At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
kernel_fpu_call(func, arg);
And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
All that being said, why are these frames so large? It sounds like something may be spilling that ought not to.
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-21 3:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Arnd Bergmann, Ard Biesheuvel, Eric Biggers, LKML, Netdev,
Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
Samuel Neves, Andrew Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <20180921031255.GB11109@lunn.ch>
Hi Andrew,
On Fri, Sep 21, 2018 at 5:13 AM Andrew Lunn <andrew@lunn.ch> wrote:
> Do you any stack usage information?
>
> A VPN can be at the bottom of some deep stack calls. Swap on NFS over
> the VPN? If you have one frame of 1K, you might be O.K. But if you
> have a few of these, i can see there might be issues of overflowing
> the stack.
It's pretty small, and most of the work is punted onto workqueues
pretty early, so the depth isn't as much of an issue as it would be if
I did everything from ndo_start_xmit. I've done some stack tracking
using the debug option for that, and it's generally far from the
deepest thing running.
Jason
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Andrew Lunn @ 2018-09-21 3:12 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Arnd Bergmann, Ard Biesheuvel, Eric Biggers, LKML, Netdev,
Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
Samuel Neves, Andrew Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <CAHmME9rJq=q7xj_E_84GN5FuzSW2KdP4Ch=y527Lem0ME4GnLg@mail.gmail.com>
On Fri, Sep 21, 2018 at 02:11:43AM +0200, Jason A. Donenfeld wrote:
> Hey Arnd,
>
> On Thu, Sep 20, 2018 at 6:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > Right, if you hit a stack requirement like this, it's usually the compiler
> > doing something bad, not just using too much stack but also generating
> > rather slow object code in the process. It's better to fix the bug by
> > optimizing the code to not spill registers to the stack.
> >
> > In the long run, I'd like to reduce the stack frame size further, so
> > best assume that anything over 1024 bytes (on 32-bit) or 1280 bytes
> > (on 64-bit) is a bug in the code, and stay below that.
> >
> > For prototyping, you can just mark the broken functions individually
> > by setting the warning limit for a specific function that is known to
> > be misoptimized by the compiler (with a comment about which compiler
> > and architectures are affected), but not override the limit for the
> > entire file.
>
> Thanks for the explanation. Fortunately in my case, the issues were
> trivially fixable to get it under 1024/1280. (By the way, why does
> 64-bit have a slightly larger stack frame? To account for 32 pointers
> taking double the space or something?) That will be rectified in v6.
Hi Jason
Do you any stack usage information?
A VPN can be at the bottom of some deep stack calls. Swap on NFS over
the VPN? If you have one frame of 1K, you might be O.K. But if you
have a few of these, i can see there might be issues of overflowing
the stack.
Andrew
^ 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