* Re: [PATCH net-next] net: ipv6: Add early demux handler for UDP unicast
From: David Miller @ 2017-04-18 15:16 UTC (permalink / raw)
To: simon.horman; +Cc: eric.dumazet, subashab, netdev, netdev-owner
In-Reply-To: <20170418080902.GA25804@vergenet.net>
From: Simon Horman <simon.horman@netronome.com>
Date: Tue, 18 Apr 2017 17:09:04 +0900
> On Wed, Mar 08, 2017 at 11:22:01AM -0800, Eric Dumazet wrote:
>> On Wed, 2017-03-08 at 12:11 -0700, Subash Abhinov Kasiviswanathan wrote:
>> > On 2017-03-08 11:40, Eric Dumazet wrote:
>> > > Well, this 'optimization' actually hurts when UDP sockets are not
>> > > connected, since this adds an extra cache line miss per incoming
>> > > packet.
>> > >
>> > > (DNS servers for example)
>> >
>> > Hi Eric
>> >
>> > Thanks for your comments. Would it be preferable to disable early demux
>> > for the
>> > servers with large unconnected workloads in that case?
>>
>> Well, many servers handle both TCP and UDP.
>>
>> For TCP, there is no question about early demux, this is definitely a
>> win.
>>
>> We probably should have one sysctl to enable TCP early demux, one for
>> UDP early demux.
>
> If early demux is a clear win for TCP then I wonder if it is
> unnecessary and by some leap also undesirable to have a configuration
> knob for that case.
For forwarding workloads it is pure overhead since the early demux will
never find a local socket, and therefore it is wasted work.
^ permalink raw reply
* Re: Case for reusing netlink PADs WAS(Re: [PATCH net-next 1/1] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: David Miller @ 2017-04-18 15:25 UTC (permalink / raw)
To: jhs; +Cc: eric.dumazet, jiri, netdev, xiyou.wangcong
In-Reply-To: <560eeb0b-7a34-484a-e4a1-c637b31eba59@mojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Tue, 18 Apr 2017 08:48:57 -0400
> For the patches I posted, I will work on getting an attribute based
> variant of the patches out - but i wanted to have this discussion a
> little more if you bear with me.
>
> Netlink is a wire protocol. When a protocol is defined with rules such
> as alignment (which lead to explicit padding) then those are
> equivalent
> to "reserved bits" in standard wire protocols. Good practise is:
> all sender zero those bits(MBZ); and all receivers must ignore them
> unless they wish to interpret them. Not everyone follows these rules
> (I remember the havoc ECN caused when TCP/IP started using the
> different
> reserved fields).
>
> For our case it is _very sad_ that someone actually explicitly defined
> pads - in my opinion for no other purpose other than reuse and then
> we say we cant use them after.
Unless you define the field to have meaning from the beginning and
truly _ENFORCE_ that meaning from the start, you cannot reuse the
field later.
So, for example, if we enforced the padding fields to be zero from day
one, and the kernel rejected non-zero values, then you could start to
consider reusing them later. Because you have %100 certainty that
existing applications fill the field in with zero.
But that is not the case here.
All of your "on the wire protocol" talk is meaningless because we
didn't do that. On the wire protocols enforce undefined and reserved
fields to meet certain requirements. We do not, in general, do that
with netlink.
This is why it is important to very carefully think ahead and define
the initial netlink operation structures fully.
If you don't get it right, and later need to add something, just take
the safe path and just add attributes and don't even think about
messing with the existing structure.
Thanks.
^ permalink raw reply
* Re: [PATCH net] selftests/net: Fixes psock_fanout CBPF test case
From: Sowmini Varadhan @ 2017-04-18 15:26 UTC (permalink / raw)
To: Mike Maloney; +Cc: netdev, davem, Mike Maloney
In-Reply-To: <20170418151416.17858-1-maloneykernel@gmail.com>
On (04/18/17 11:14), Mike Maloney wrote:
> Change 'psock_fanout' to use SOCK_RAW so that the CBPF program used with
> SO_ATTACH_FILTER can examine the entire frame. Create a new CBPF
> program for use with PACKET_FANOUT_DATA which ignores the header, as it
> cannot see the ethernet header.
Fix look good to me, but could you please also add the bpf_asm input
as a comment to the C code, in case we want to to read/extend this
down the road?
--Sowmini
^ permalink raw reply
* Re: Case for reusing netlink PADs WAS(Re: [PATCH net-next 1/1] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: David Miller @ 2017-04-18 15:29 UTC (permalink / raw)
To: jiri; +Cc: jhs, eric.dumazet, netdev, xiyou.wangcong
In-Reply-To: <20170418131656.GG1871@nanopsycho.orion>
From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 18 Apr 2017 15:16:56 +0200
> It's ugly. Plus user may pass garbage in pads from current apps.
%100 agreed.
> Why you don't just use new attributes? I don't get it. I still
> don't understand why people feel need to do struct style message
> passing in TLV-designed Netlink interface...
Also agreed, I don't see why it's such a big deal.
Once we define a structure, let's set it in stone and don't try to
modify it's layout or meaning. It is the only safe approach.
We made netlink have attributes exactly for this reason.
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:42 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, David Laight, xen-devel
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig,
fcoe-devel@open-fcoe.org, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
Ming Lin, "intel-gfx@list
In-Reply-To: <20170418142723.GA27133@char.us.oracle.com>
On 18/04/17 08:27 AM, Konrad Rzeszutek Wilk wrote:
> Interesting that you didn't CC any of the maintainers. Could you
> do that in the future please?
Please read the cover letter. The distribution list for the patchset
would have been way too large to cc every maintainer (even as limited as
it was, I had mailing lists yelling at me). My plan was to get buy in
for the first patch, get it merged and resend the rest independently to
their respective maintainers. Of course, though, I'd be open to other
suggestions.
>>>
>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>> ---
>>> drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++------
>>> 1 file changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index 5067a0a..7dcf41d 100644
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
>>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
>>>
>>> if (setup.need_copy) {
>>> - setup.bvec_off = sg->offset;
>>> - setup.bvec_data = kmap_atomic(sg_page(sg));
>>> + setup.bvec_off = 0;
>>> + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
>>> + if (IS_ERR(setup.bvec_data)) {
>>> + /*
>>> + * This should really never happen unless
>>> + * the code is changed to use memory that is
>>> + * not mappable in the sg. Seeing there is a
>>> + * questionable error path out of here,
>>> + * we WARN.
>>> + */
>>> + WARN(1, "Non-mappable memory used in sg!");
>>> + return 1;
>>> + }
>> ...
>>
>> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
>> inside sg_map().
Thanks, that's a good suggestion. I'll make the change for v2.
Logan
^ permalink raw reply
* Re: [PATCH 05/22] drm/i915: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:44 UTC (permalink / raw)
To: Christoph Hellwig, Martin K. Petersen, Sagi Grimberg, Jens Axboe,
Tejun Heo, Greg Kroah-Hartman, Dan Williams, Ross Zwisler,
Matthew Wilcox, Sumit Semwal, Ming Lin, linux-kernel,
linux-crypto, linux-media, dri-devel, linaro-mm-sig, intel-gfx,
linux-raid, linux-mmc, linux-nvme, linux-nvdimm, linux-scsi,
fcoe-devel, open-iscsi, megaraidlinux.pdl, sparmaintainer, devel
In-Reply-To: <20170418064427.r5ewu3p66p2zwdru@phenom.ffwll.local>
On 18/04/17 12:44 AM, Daniel Vetter wrote:
> On Thu, Apr 13, 2017 at 04:05:18PM -0600, Logan Gunthorpe wrote:
>> This is a single straightforward conversion from kmap to sg_map.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Probably makes sense to merge through some other tree, but please be aware
> of the considerable churn rate in i915 (i.e. make sure your tree is in
> linux-next before you send a pull request for this). Plane B would be to
> get the prep patch in first and then merge the i915 conversion one kernel
> release later.
Yes, as per what I said in my cover letter, I was leaning towards a
"Plan B" style approach.
Logan
^ permalink raw reply
* Re: [PATCH net-next v2] net: ipv6: Fix UDP early demux lookup with udp_l3mdev_accept=0
From: David Ahern @ 2017-04-18 15:46 UTC (permalink / raw)
To: Subash Abhinov Kasiviswanathan, YUAN Linyu
Cc: davem, netdev, rshearma, eric.dumazet, Eric Dumazet
In-Reply-To: <2a12c7c44ddd3e372a84db9318a40c98@codeaurora.org>
On 4/17/17 7:07 PM, Subash Abhinov Kasiviswanathan wrote:
>>> + break;
>> I think break here should remove ?
>
> Hi Yuan
>
> This is similar to __udp4_lib_demux_lookup where we need to check if the
> first
> socket is an exact match or break since chains maybe long.
>
I suggest adding the same comment as __udp4_lib_demux_lookup; it does
look odd.
^ permalink raw reply
* Re: net/ipv6: slab-out-of-bounds read in seg6_validate_srh
From: Andrey Konovalov @ 2017-04-18 15:46 UTC (permalink / raw)
To: David Lebrun
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Cong Wang,
Eric Dumazet, Dmitry Vyukov, Kostya Serebryany, syzkaller
In-Reply-To: <bd8d4b77-ee74-73a7-55af-404a1b666460@uclouvain.be>
On Tue, Apr 18, 2017 at 5:16 PM, David Lebrun <david.lebrun@uclouvain.be> wrote:
> On 04/18/2017 04:54 PM, Andrey Konovalov wrote:
>> Hi,
>>
>> I've got the following error report while fuzzing the kernel with syzkaller.
>>
>> On commit 4f7d029b9bf009fbee76bb10c0c4351a1870d2f3 (4.11-rc7).
>>
>> A reproducer and .config are attached.
>>
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in seg6_validate_srh+0x203/0x220
>> net/ipv6/seg6.c:57 at addr ffff88006a759608
>> Read of size 1 by task syz-executor4/2627
>
> Thanks for the report. It seems to happen when the trailing data is less
> than sizeof(struct sr6_tlv). The following (untested) patch should fix
> the issue, I'll test it and submit it properly if it works.
Hi David,
This fixes the bug.
Thanks!
Tested-by: Andrey Konovalov <andreyknvl@google.com>
>
> diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> index a855eb3..5f44ffe 100644
> --- a/net/ipv6/seg6.c
> +++ b/net/ipv6/seg6.c
> @@ -53,6 +53,9 @@ bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
> struct sr6_tlv *tlv;
> unsigned int tlv_len;
>
> + if (trailing < sizeof(*tlv))
> + return false;
> +
> tlv = (struct sr6_tlv *)((unsigned char *)srh + tlv_offset);
> tlv_len = sizeof(*tlv) + tlv->len;
>
>
>
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Konrad Rzeszutek Wilk @ 2017-04-18 15:50 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, xen-devel, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig,
fcoe-devel@open-fcoe.org, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
Ming Lin
In-Reply-To: <7930aa93-6106-e12f-ba76-e2771d4ec2dc@deltatee.com>
On Tue, Apr 18, 2017 at 09:42:20AM -0600, Logan Gunthorpe wrote:
>
>
> On 18/04/17 08:27 AM, Konrad Rzeszutek Wilk wrote:
> > Interesting that you didn't CC any of the maintainers. Could you
> > do that in the future please?
>
> Please read the cover letter. The distribution list for the patchset
> would have been way too large to cc every maintainer (even as limited as
> it was, I had mailing lists yelling at me). My plan was to get buy in
I am not sure if you know, but you can add on each patch the respective
maintainer via 'CC'. That way you can have certain maintainers CCed only
on the subsystems they cover. You put it after (or before) your SoB and
git send-email happilly picks it up.
It does mean that for every patch you have to run something like this:
$ more add_cc
#!/bin/bash
git diff HEAD^.. > /tmp/a
echo "---"
scripts/get_maintainer.pl --no-l /tmp/a | while read file
do
echo "Cc: $file"
done
Or such.
> for the first patch, get it merged and resend the rest independently to
> their respective maintainers. Of course, though, I'd be open to other
> suggestions.
>
> >>>
> >>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >>> ---
> >>> drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++------
> >>> 1 file changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >>> index 5067a0a..7dcf41d 100644
> >>> --- a/drivers/block/xen-blkfront.c
> >>> +++ b/drivers/block/xen-blkfront.c
> >>> @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
> >>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >>>
> >>> if (setup.need_copy) {
> >>> - setup.bvec_off = sg->offset;
> >>> - setup.bvec_data = kmap_atomic(sg_page(sg));
> >>> + setup.bvec_off = 0;
> >>> + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
> >>> + if (IS_ERR(setup.bvec_data)) {
> >>> + /*
> >>> + * This should really never happen unless
> >>> + * the code is changed to use memory that is
> >>> + * not mappable in the sg. Seeing there is a
> >>> + * questionable error path out of here,
> >>> + * we WARN.
> >>> + */
> >>> + WARN(1, "Non-mappable memory used in sg!");
> >>> + return 1;
> >>> + }
> >> ...
> >>
> >> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
> >> inside sg_map().
>
> Thanks, that's a good suggestion. I'll make the change for v2.
>
> Logan
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: Eric Dumazet @ 2017-04-18 15:51 UTC (permalink / raw)
To: James Hughes; +Cc: netdev, Steve Glendinning, Microchip Linux Driver Support
In-Reply-To: <20170418144857.7810-1-james.hughes@raspberrypi.org>
On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
> The driver was failing to check that the SKB wasn't cloned
> before adding checksum data or adding header data.
> Replace existing handling to extend the buffer with
> skb_cow. Don't use skb_cow_head as the sw checksum
> code modifies the data portion.
>
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> drivers/net/usb/smsc95xx.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index df60c98..04f6397 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
>
> - if (skb_headroom(skb) < overhead) {
> - struct sk_buff *skb2 = skb_copy_expand(skb,
> - overhead, 0, flags);
> - dev_kfree_skb_any(skb);
> - skb = skb2;
> - if (!skb)
> - return NULL;
> + /* Make writable and expand space by overhead if required */
> + if (skb_cow(skb, overhead)) {
> + return NULL;
> }
Note that this patch will probably force a copy of all locally generated
TCP packets.
For them skb_cloned(skb) is true.
I do believe skb_cow_head() would be better, since TCP stack uses the
__skb_header_release() helper to tell lower stacks they can write the
header part, even on a clone.
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: David Miller @ 2017-04-18 15:55 UTC (permalink / raw)
To: eric.dumazet; +Cc: james.hughes, netdev, steve.glendinning, UNGLinuxDriver
In-Reply-To: <1492530711.10587.125.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 18 Apr 2017 08:51:51 -0700
> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>> The driver was failing to check that the SKB wasn't cloned
>> before adding checksum data or adding header data.
>> Replace existing handling to extend the buffer with
>> skb_cow. Don't use skb_cow_head as the sw checksum
>> code modifies the data portion.
>>
>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>> ---
>> drivers/net/usb/smsc95xx.c | 10 +++-------
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index df60c98..04f6397 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>> /* We do not advertise SG, so skbs should be already linearized */
>> BUG_ON(skb_shinfo(skb)->nr_frags);
>>
>> - if (skb_headroom(skb) < overhead) {
>> - struct sk_buff *skb2 = skb_copy_expand(skb,
>> - overhead, 0, flags);
>> - dev_kfree_skb_any(skb);
>> - skb = skb2;
>> - if (!skb)
>> - return NULL;
>> + /* Make writable and expand space by overhead if required */
>> + if (skb_cow(skb, overhead)) {
>> + return NULL;
>> }
>
> Note that this patch will probably force a copy of all locally generated
> TCP packets.
>
> For them skb_cloned(skb) is true.
>
> I do believe skb_cow_head() would be better, since TCP stack uses the
> __skb_header_release() helper to tell lower stacks they can write the
> header part, even on a clone.
Agreed.
^ permalink raw reply
* Re: [PATCH net] selftests/net: Fixes psock_fanout CBPF test case
From: Mike Maloney @ 2017-04-18 15:56 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: netdev, davem, Mike Maloney
In-Reply-To: <20170418152616.GE18699@oracle.com>
On Tue, Apr 18, 2017 at 11:26 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (04/18/17 11:14), Mike Maloney wrote:
>> Change 'psock_fanout' to use SOCK_RAW so that the CBPF program used with
>> SO_ATTACH_FILTER can examine the entire frame. Create a new CBPF
>> program for use with PACKET_FANOUT_DATA which ignores the header, as it
>> cannot see the ethernet header.
>
> Fix look good to me, but could you please also add the bpf_asm input
> as a comment to the C code, in case we want to to read/extend this
> down the road?
>
> --Sowmini
>
I am not 100% sure what you are asking for, as the instructions you
can feed to bpf_asm are already commented to the right of the program.
-Mike
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:59 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, xen-devel, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
intel-gfx@lists.freedesktop.org, "sparma
In-Reply-To: <20170418155020.GF12001@char.us.oracle.com>
On 18/04/17 09:50 AM, Konrad Rzeszutek Wilk wrote:
> I am not sure if you know, but you can add on each patch the respective
> maintainer via 'CC'. That way you can have certain maintainers CCed only
> on the subsystems they cover. You put it after (or before) your SoB and
> git send-email happilly picks it up.
Yes, but I've seen some maintainers complain when they receive a patch
with no context (ie. cover letter and first patch). So I chose to do it
this way. I expect in this situation, no matter what you do, someone is
going to complain about the approach chosen.
Thanks anyway for the tip.
Logan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH net-next v2] net: ipv6: Fix UDP early demux lookup with udp_l3mdev_accept=0
From: David Ahern @ 2017-04-18 15:59 UTC (permalink / raw)
To: Subash Abhinov Kasiviswanathan, davem, netdev, rshearma,
eric.dumazet
Cc: Eric Dumazet
In-Reply-To: <1492471495-13073-1-git-send-email-subashab@codeaurora.org>
On 4/17/17 5:24 PM, Subash Abhinov Kasiviswanathan wrote:
> David Ahern reported that 5425077d73e0c ("net: ipv6: Add early demux
> handler for UDP unicast") breaks udp_l3mdev_accept=0 since early
> demux for IPv6 UDP was doing a generic socket lookup which does not
> require an exact match. Fix this by making UDPv6 early demux match
> connected sockets only.
>
> v1->v2: Take reference to socket after match as suggested by Eric
>
> Fixes: 5425077d73e0c ("net: ipv6: Add early demux handler for UDP unicast")
> Reported-by: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> net/ipv6/udp.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
Besides adding the comment before the break, this looks fine to me.
Acked-by: David Ahern <dsa@cumulusnetworks.com>
Tested-by: David Ahern <dsa@cumulusnetworks.com>
If the only change for v3 is the comment before the break, please keep
the above with the new patch.
^ permalink raw reply
* [PATCH net] ipv6: sr: fix out-of-bounds access in SRH validation
From: David Lebrun @ 2017-04-18 15:59 UTC (permalink / raw)
To: netdev; +Cc: David Lebrun, Andrey Konovalov
This patch fixes an out-of-bounds access in seg6_validate_srh() when the
trailing data is less than sizeof(struct sr6_tlv).
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: David Lebrun <david.lebrun@uclouvain.be>
---
net/ipv6/seg6.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index a855eb3..5f44ffe 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -53,6 +53,9 @@ bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len)
struct sr6_tlv *tlv;
unsigned int tlv_len;
+ if (trailing < sizeof(*tlv))
+ return false;
+
tlv = (struct sr6_tlv *)((unsigned char *)srh + tlv_offset);
tlv_len = sizeof(*tlv) + tlv->len;
--
2.10.2
^ permalink raw reply related
* Re: net/ipv4: use-after-free in ip_queue_xmit
From: Cong Wang @ 2017-04-18 16:05 UTC (permalink / raw)
To: Andrey Konovalov
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Eric Dumazet,
Dmitry Vyukov, Kostya Serebryany, syzkaller
In-Reply-To: <CAAeHK+wYLwZqGWjBNbw6mbvTiHCeXC91dYmMY410YwprU8OG1w@mail.gmail.com>
On Tue, Apr 18, 2017 at 5:15 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> Yes, I don't have this field in the rtable struct.
>
> I'm on 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
>
> I also don't see it in the cross reference:
> http://lxr.free-electrons.com/source/include/net/route.h#L51
>
It is provided by my patch: https://patchwork.ozlabs.org/patch/747556/
which means you applied an incomplete patch... :-/
^ permalink raw reply
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: James Hughes @ 2017-04-18 16:16 UTC (permalink / raw)
To: David Miller
Cc: Eric Dumazet, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <20170418.115546.921863837348166438.davem@davemloft.net>
On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 18 Apr 2017 08:51:51 -0700
>
>> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>>> The driver was failing to check that the SKB wasn't cloned
>>> before adding checksum data or adding header data.
>>> Replace existing handling to extend the buffer with
>>> skb_cow. Don't use skb_cow_head as the sw checksum
>>> code modifies the data portion.
>>>
>>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>>> ---
>>> drivers/net/usb/smsc95xx.c | 10 +++-------
>>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>>> index df60c98..04f6397 100644
>>> --- a/drivers/net/usb/smsc95xx.c
>>> +++ b/drivers/net/usb/smsc95xx.c
>>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>>> /* We do not advertise SG, so skbs should be already linearized */
>>> BUG_ON(skb_shinfo(skb)->nr_frags);
>>>
>>> - if (skb_headroom(skb) < overhead) {
>>> - struct sk_buff *skb2 = skb_copy_expand(skb,
>>> - overhead, 0, flags);
>>> - dev_kfree_skb_any(skb);
>>> - skb = skb2;
>>> - if (!skb)
>>> - return NULL;
>>> + /* Make writable and expand space by overhead if required */
>>> + if (skb_cow(skb, overhead)) {
>>> + return NULL;
>>> }
>>
>> Note that this patch will probably force a copy of all locally generated
>> TCP packets.
>>
>> For them skb_cloned(skb) is true.
>>
>> I do believe skb_cow_head() would be better, since TCP stack uses the
>> __skb_header_release() helper to tell lower stacks they can write the
>> header part, even on a clone.
>
> Agreed.
I'm happy to work it as you see fit - you know this code far better than I do.
Our reading of the code is that the software checksum path is
modifying the data rather than just adding a header. Based on the
description of skb_cow_head it therefore isn't appropriate. If that
isn't a concern in reality then skb_cow_head is fine and I'll make a
V2 patchset.
Or do we need to skb_cow if doing the software checksum, but
skb_cow_head normally? That can be done instead but requires a
slightly larger change.
The failure case we were seeing was with a bridged network using
SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
making an SKB clone of broadcasts for the two interfaces, and then
both drivers were adding headers without checking skb_cloned(skb)
first, hence trampling on each other. For small packets the SMSC95xx
driver will be computing the software checksum and writing it in to
the data, so the wifi driver will also be seeing it. For many drivers
that probably won't matter, but is that always true?
(Patches for the Broadcom wifi driver will be coming once we've worked
out the best way of fixing this - there is no error path easily
available if the skb_cow_head call fails).
James
^ permalink raw reply
* Re: [PATCH next RFC] net: sched: mirred: move xmit to tasklet
From: Eric Dumazet @ 2017-04-18 16:18 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev
In-Reply-To: <20170418130331.14268-1-fw@strlen.de>
On Tue, 2017-04-18 at 15:03 +0200, Florian Westphal wrote:
> mirred is prone to deadlocks as it invokes dev_queue_xmit while
> holding one or more qdisc locks.
>
> Avoid lock recursions by moving tx context to a tasklet.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> This a stab at removing the lock recursions discussed during netconf.
>
> Taking the cost of the tasklet appears to be the only solution;
> i tried to use a percpu 'history' instead but its not clear to
> me that this avoids all corner cases.
>
> While this patch doesn't avoid loops we don't hang the kernel
> anymore and removing the 'looping' filter makes things calm
> down again (there are also other ways to create such loops anyway,
> including use of a cable... )
>
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index 1b5549ababd4..3dd61eba4741 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -17,6 +17,7 @@
> #include <linux/string.h>
> #include <linux/errno.h>
> #include <linux/skbuff.h>
> +#include <linux/skb_array.h>
> #include <linux/rtnetlink.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -25,10 +26,19 @@
> #include <net/net_namespace.h>
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> +#include <net/dst.h>
> #include <linux/tc_act/tc_mirred.h>
> #include <net/tc_act/tc_mirred.h>
>
> +#define MIRRED_TXLEN 512
Using an skb array looks overkill to me, especially if using per cpu
queue. A standard skb list should be good enough ?
> +static void mirred_cleanup_pcpu(void)
> +{
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct mirred_tx_data *data;
> +
> + data = per_cpu_ptr(&mirred_tx_data, cpu);
> +
> + skb_array_cleanup(&data->skb_array);
This wont do the dev_put() on skb->dev
> + tasklet_kill(&data->mirred_tasklet);
You might need to kill the tasklet _before_ doing the cleanup ?
> + }
> +}
> +
^ permalink raw reply
* Re: Use of skb_unclone in drivers
From: Florian Fainelli @ 2017-04-18 16:34 UTC (permalink / raw)
To: James Hughes, netdev
In-Reply-To: <CAE_XsMLghPZTe69-eUF322U53gZcUfXzu-0VScf7yNCrLkgOBA@mail.gmail.com>
On 04/18/2017 01:34 AM, James Hughes wrote:
> Thanks,
(please don't top-post on netdev)
>
> Quick check on that function - very similar to unclone which we know
> ameliorates the issue, just has the headroom parameter rather than
> priority. So clearly the right way to go. Just one more question I
> hope - where is the appropriate place for the skb_cow_head call in
> the driver - at the main entry point, or closer or in the function(s)
> that may alter the header itself?
You should place the call to skb_cow_head() in the same function(s) that
do(es) the header(s) modifications, that way it's clear to the reader
what the intent is, and the code is reasonably easy to audit.
>
> James
>
> (Apologies to Eric for originally sending him this message rather than
> to netdev - was doing it all from a phone and screwed it up)
>
> On 17 April 2017 at 17:07, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Mon, 2017-04-17 at 16:02 +0100, James Hughes wrote:
>>> Netdevs,
>>>
>>> We have recently got to the bottom of an issue which we have been
>>> encountering on a Raspberry Pi being used as an access point, and we
>>> need a bit of advice on the correct way of fixing the issue.
>>>
>>> The set up is a Raspberry Pi 3 running hostapd on its inbuilt wireless
>>> adaptor (Brcm43438 ), bridged to the built in ethernet adaptor
>>> (smsc9514). Using the standard drivers for these devices as of 4.9
>>> (looking at 4.11, no changes noted that would affect the issue. I will
>>> be trying the latest kernel once I get back in to the office).
>>>
>>> We were encountering an error in the Brcm Wireless driver that after
>>> investigation was a skb_buff being corrupted by an action in the smsc
>>> Ethernet driver. Further digging shows that the bridge was cloning an
>>> incoming skb from the Ethernet, then sending it out to both the
>>> Ethernet and wlan ports. This mean that both the Ethernet driver and
>>> the wireless driver were looking at the same physical data, and one or
>>> both were altering that data in different ways (varied headers) ,
>>> which mean that headers were corrupted. This was only happening on
>>> broadcast packets.
>>>
>>> We can fix the issue by, in the drivers, using skb_unclone in
>>> appropriate places in the driver.
>>>
>>> So now to the questions. Is adding the unclone to the drivers the
>>> correct way of dealing with this issue? Examining other drivers shows
>>> that unclone is not particularly common, presumably in many cases,
>>> where the driver does not alter the skb, it doesn't matter. However,
>>> in any case where a driver may add header information to the skb (as
>>> is the case with the Brcm wireless driver), when the incoming skb has
>>> been cloned, the results must surely be undetermined unless an unclone
>>> is performed.
>>>
>>> Or, is the bridging code making a mistake by cloning the skb and
>>> passing it to multiple recipients?
>>>
>>
>>
>> bridge code is fine.
>>
>> Problem is that some drivers lack calls to skb_cow_head() before they
>> mess with headers.
>>
>>
>>
>>
>>
>>
>
>
>
--
Florian
^ permalink raw reply
* Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers
From: Florian Fainelli @ 2017-04-18 16:40 UTC (permalink / raw)
To: Niklas Cassel, Giuseppe CAVALLARO, Andrew Lunn, Yegor Yefremov
Cc: netdev, linux-omap@vger.kernel.org, Grygorii Strashko,
N, Mugunthan V, Rami Rosen, Fabrice GASNIER, rmk+kernel
In-Reply-To: <a45177c1-cd2b-46a3-7858-753ad3f28ae7@axis.com>
On 04/18/2017 06:23 AM, Niklas Cassel wrote:
> On 01/04/2017 03:33 PM, Florian Fainelli wrote:
>> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>>> Peppe, any thoughts on this?
>>>>
>>>> I share what you say.
>>>>
>>>> In sum, the EEE management inside the stmmac is:
>>>>
>>>> - the driver looks at own HW cap register if EEE is supported
>>>>
>>>> (indeed the user could keep disable EEE if bugged on some HW
>>>> + Alex, Fabrice: we had some patches for this to propose where we
>>>> called the phy_ethtool_set_eee to disable feature at phy
>>>> level
>>>>
>>>> - then the stmmac asks PHY layer to understand if transceiver and
>>>> partners are EEE capable.
>>>>
>>>> - If all matches the EEE is actually initialized.
>>>>
>>>> the logic above should be respected when use ethtool, hmm, I will
>>>> check the stmmac_ethtool_op_set_eee asap.
>>>>
>>>> Hoping this is useful
>>>
>>> This is definitively useful, the only part that I am struggling to
>>> understand in phy_init_eee() is this:
>>>
>>> eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>>> MDIO_MMD_AN);
>>> if (eee_adv <= 0)
>>> goto eee_exit_err;
>>>
>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>>> the time we call phy_init_eee(), then we cannot complete the EEE
>>> configuration at the PHY level, and presumably we should abort the EEE
>>> configuration at the MAC level.
>>>
>>> While this condition makes sense if e.g: you are re-negotiating the link
>>> with your partner for instance and if EEE was already advertised, the
>>> very first time this function is called, it seems to be like we should
>>> skip the check, because phy_init_eee() should actually tell us if, as a
>>> result of a successful check, we should be setting EEE as something we
>>> advertise?
>>>
>>> Do you remember what was the logic behind this check when you added it?
>>
>> Peppe, can you remember why phy_init_eee() was written in a way that you
>> need to have already locally advertised EEE for the function to
>> successfully return? Thank you!
>>
>
> I'm curious about this as well.
>
> I can get EEE to work with stmmac, but to be able to turn EEE on,
> I need to set eee advertise via ethtool first.
> (Tested with 2 different PHYs from different vendors, with their
> PHY specific driver enabled.)
>
> Is this the same for all PHYs or are there certain PHYs/PHY drivers
> that actually advertise eee by default?
It depends on whether the PHY driver takes care of the EEE advertisement
part for your or not, most drivers probably don't do that.
> (From reading this mail thread there seems to be a suggestion that
> the broadcom PHY driver might advertise eee by default.)
As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do
advertise EEE by default in order to validate the first check done in
phy_init_eee(), but that's the only reason really.
Since we have not been able to get a straight answer from Peppe about
why there is this initial check, I think the cleanest path moving
forward is the following:
- rename phy_init_eee() into something like: phy_can_do_eee() and remove
the first check on whether EEE is already advertised because that's
precisely what we are trying to determine with this function
- Ethernet MAC drivers keep calling phy_can_do_eee() (formerly
phy_init_eee()) during their adjust_link callback in order to
re-negotiate EEE with their link partner, just like they should call
phy_ethtool_set_eee() to really enable EEE the first time they want to
enable EEE with the link partner
- remove the part from phy_init_eee() that tries to stop the PHY TX
clock and provide a set of helpers: phy_can_stop_tx_clk() and
phy_set_stop_tx_clk() which will take care of that
Does that look reasonable?
--
Florian
^ permalink raw reply
* Re: Use of skb_unclone in drivers
From: James Hughes @ 2017-04-18 16:43 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev
In-Reply-To: <75112dc9-dd75-3473-b39d-0687cdff3391@gmail.com>
On 18 April 2017 at 17:34, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 04/18/2017 01:34 AM, James Hughes wrote:
>> Thanks,
>
> (please don't top-post on netdev)
>
Trying not to! New to this.
>>
>> Quick check on that function - very similar to unclone which we know
>> ameliorates the issue, just has the headroom parameter rather than
>> priority. So clearly the right way to go. Just one more question I
>> hope - where is the appropriate place for the skb_cow_head call in
>> the driver - at the main entry point, or closer or in the function(s)
>> that may alter the header itself?
>
> You should place the call to skb_cow_head() in the same function(s) that
> do(es) the header(s) modifications, that way it's clear to the reader
> what the intent is, and the code is reasonably easy to audit.
>
OK, thanks - have done that and posted a patch for the SMSC driver which
others are reviewing. Still trying to sort out the Brcm wireless driver which
is lacking any sane error path should the skb_cow_head function fail in
the same function where the header modification is made.
>>
>> James
>>
>> (Apologies to Eric for originally sending him this message rather than
>> to netdev - was doing it all from a phone and screwed it up)
>>
>> On 17 April 2017 at 17:07, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> On Mon, 2017-04-17 at 16:02 +0100, James Hughes wrote:
>>>> Netdevs,
>>>>
>>>> We have recently got to the bottom of an issue which we have been
>>>> encountering on a Raspberry Pi being used as an access point, and we
>>>> need a bit of advice on the correct way of fixing the issue.
>>>>
>>>> The set up is a Raspberry Pi 3 running hostapd on its inbuilt wireless
>>>> adaptor (Brcm43438 ), bridged to the built in ethernet adaptor
>>>> (smsc9514). Using the standard drivers for these devices as of 4.9
>>>> (looking at 4.11, no changes noted that would affect the issue. I will
>>>> be trying the latest kernel once I get back in to the office).
>>>>
>>>> We were encountering an error in the Brcm Wireless driver that after
>>>> investigation was a skb_buff being corrupted by an action in the smsc
>>>> Ethernet driver. Further digging shows that the bridge was cloning an
>>>> incoming skb from the Ethernet, then sending it out to both the
>>>> Ethernet and wlan ports. This mean that both the Ethernet driver and
>>>> the wireless driver were looking at the same physical data, and one or
>>>> both were altering that data in different ways (varied headers) ,
>>>> which mean that headers were corrupted. This was only happening on
>>>> broadcast packets.
>>>>
>>>> We can fix the issue by, in the drivers, using skb_unclone in
>>>> appropriate places in the driver.
>>>>
>>>> So now to the questions. Is adding the unclone to the drivers the
>>>> correct way of dealing with this issue? Examining other drivers shows
>>>> that unclone is not particularly common, presumably in many cases,
>>>> where the driver does not alter the skb, it doesn't matter. However,
>>>> in any case where a driver may add header information to the skb (as
>>>> is the case with the Brcm wireless driver), when the incoming skb has
>>>> been cloned, the results must surely be undetermined unless an unclone
>>>> is performed.
>>>>
>>>> Or, is the bridging code making a mistake by cloning the skb and
>>>> passing it to multiple recipients?
>>>>
>>>
>>>
>>> bridge code is fine.
>>>
>>> Problem is that some drivers lack calls to skb_cow_head() before they
>>> mess with headers.
>>>
>>
>
>
> --
> Florian
--
James Hughes
Principal Software Engineer,
Raspberry Pi (Trading) Ltd
^ permalink raw reply
* [PATCH net-next 0/2] tcp: address two poll() flakes
From: Eric Dumazet @ 2017-04-18 16:45 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
Some packetdrill tests are failing when host kernel is using ASAN
or other debugging infrastructure.
I was able to fix the flakes by making sure we were not
sending wakeup events too soon.
Eric Dumazet (2):
tcp: remove poll() flakes when receiving RST
tcp: remove poll() flakes with FastOpen
net/ipv4/tcp_input.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
--
2.12.2.762.g0e3151a226-goog
^ permalink raw reply
* [PATCH net-next 1/2] tcp: remove poll() flakes when receiving RST
From: Eric Dumazet @ 2017-04-18 16:45 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
In-Reply-To: <20170418164552.29261-1-edumazet@google.com>
When a RST packet is processed, we send two wakeup events to interested
polling users.
First one by a sk->sk_error_report(sk) from tcp_reset(),
followed by a sk->sk_state_change(sk) from tcp_done().
Depending on machine load and luck, poll() can either return POLLERR,
or POLLIN|POLLOUT|POLLERR|POLLHUP (this happens on 99 % of the cases)
This is probably fine, but we can avoid the confusion by reordering
things so that we have more TCP fields updated before the first wakeup.
This might even allow us to remove some barriers we added in the past.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_input.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a5838858c362cd3270296001ceaae341e9e9bf01..37e2aa925f62395cfb48145cd3a76b6afebb64b1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4008,10 +4008,10 @@ void tcp_reset(struct sock *sk)
/* This barrier is coupled with smp_rmb() in tcp_poll() */
smp_wmb();
+ tcp_done(sk);
+
if (!sock_flag(sk, SOCK_DEAD))
sk->sk_error_report(sk);
-
- tcp_done(sk);
}
/*
--
2.12.2.762.g0e3151a226-goog
^ permalink raw reply related
* [PATCH net-next 2/2] tcp: remove poll() flakes with FastOpen
From: Eric Dumazet @ 2017-04-18 16:45 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
In-Reply-To: <20170418164552.29261-1-edumazet@google.com>
When using TCP FastOpen for an active session, we send one wakeup event
from tcp_finish_connect(), right before the data eventually contained in
the received SYNACK is queued to sk->sk_receive_queue.
This means that depending on machine load or luck, poll() users
might receive POLLOUT events instead of POLLIN|POLLOUT
To fix this, we need to move the call to sk->sk_state_change()
after the (optional) call to tcp_rcv_fastopen_synack()
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_input.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 37e2aa925f62395cfb48145cd3a76b6afebb64b1..341f021f02a2931cd75b2e1e71af9729fc4c7895 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5580,10 +5580,6 @@ void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
else
tp->pred_flags = 0;
- if (!sock_flag(sk, SOCK_DEAD)) {
- sk->sk_state_change(sk);
- sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
- }
}
static bool tcp_rcv_fastopen_synack(struct sock *sk, struct sk_buff *synack,
@@ -5652,6 +5648,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_fastopen_cookie foc = { .len = -1 };
int saved_clamp = tp->rx_opt.mss_clamp;
+ bool fastopen_fail;
tcp_parse_options(skb, &tp->rx_opt, 0, &foc);
if (tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr)
@@ -5755,10 +5752,15 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
tcp_finish_connect(sk, skb);
- if ((tp->syn_fastopen || tp->syn_data) &&
- tcp_rcv_fastopen_synack(sk, skb, &foc))
- return -1;
+ fastopen_fail = (tp->syn_fastopen || tp->syn_data) &&
+ tcp_rcv_fastopen_synack(sk, skb, &foc);
+ if (!sock_flag(sk, SOCK_DEAD)) {
+ sk->sk_state_change(sk);
+ sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
+ }
+ if (fastopen_fail)
+ return -1;
if (sk->sk_write_pending ||
icsk->icsk_accept_queue.rskq_defer_accept ||
icsk->icsk_ack.pingpong) {
--
2.12.2.762.g0e3151a226-goog
^ permalink raw reply related
* Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs
From: Eric Dumazet @ 2017-04-18 16:52 UTC (permalink / raw)
To: James Hughes
Cc: David Miller, netdev, Steve Glendinning,
Microchip Linux Driver Support
In-Reply-To: <CAE_XsMJZGPvENhCgyvuz48PyLk=T7L-tGoktzVBLPPYDP7oR8A@mail.gmail.com>
On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 18 Apr 2017 08:51:51 -0700
> >
> >> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
> >>> The driver was failing to check that the SKB wasn't cloned
> >>> before adding checksum data or adding header data.
> >>> Replace existing handling to extend the buffer with
> >>> skb_cow. Don't use skb_cow_head as the sw checksum
> >>> code modifies the data portion.
> >>>
> >>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> >>> ---
> >>> drivers/net/usb/smsc95xx.c | 10 +++-------
> >>> 1 file changed, 3 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> >>> index df60c98..04f6397 100644
> >>> --- a/drivers/net/usb/smsc95xx.c
> >>> +++ b/drivers/net/usb/smsc95xx.c
> >>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> >>> /* We do not advertise SG, so skbs should be already linearized */
> >>> BUG_ON(skb_shinfo(skb)->nr_frags);
> >>>
> >>> - if (skb_headroom(skb) < overhead) {
> >>> - struct sk_buff *skb2 = skb_copy_expand(skb,
> >>> - overhead, 0, flags);
> >>> - dev_kfree_skb_any(skb);
> >>> - skb = skb2;
> >>> - if (!skb)
> >>> - return NULL;
> >>> + /* Make writable and expand space by overhead if required */
> >>> + if (skb_cow(skb, overhead)) {
> >>> + return NULL;
> >>> }
> >>
> >> Note that this patch will probably force a copy of all locally generated
> >> TCP packets.
> >>
> >> For them skb_cloned(skb) is true.
> >>
> >> I do believe skb_cow_head() would be better, since TCP stack uses the
> >> __skb_header_release() helper to tell lower stacks they can write the
> >> header part, even on a clone.
> >
> > Agreed.
>
> I'm happy to work it as you see fit - you know this code far better than I do.
>
> Our reading of the code is that the software checksum path is
> modifying the data rather than just adding a header. Based on the
> description of skb_cow_head it therefore isn't appropriate. If that
> isn't a concern in reality then skb_cow_head is fine and I'll make a
> V2 patchset.
> Or do we need to skb_cow if doing the software checksum, but
> skb_cow_head normally? That can be done instead but requires a
> slightly larger change.
>
> The failure case we were seeing was with a bridged network using
> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
> making an SKB clone of broadcasts for the two interfaces, and then
> both drivers were adding headers without checking skb_cloned(skb)
> first, hence trampling on each other. For small packets the SMSC95xx
> driver will be computing the software checksum and writing it in to
> the data, so the wifi driver will also be seeing it. For many drivers
> that probably won't matter, but is that always true?
>
> (Patches for the Broadcom wifi driver will be coming once we've worked
> out the best way of fixing this - there is no error path easily
> available if the skb_cow_head call fails).
>
You misread what the driver does.
The TCP data (payload) is _not_ modified.
Only additional headers are pushed in front of the existing (Ethernet,
IP, TCP) headers.
For this, skb_cow_head() is the perfect solution.
^ 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