* Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver
From: Andrew Lunn @ 2016-03-24 22:06 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: davem, netdev
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264402F31B5@CHN-SV-EXMX02.mchp-main.com>
> It appears the dsa.c is not able to attach my underlying net
> device. And that seems to be due to it is unable to find the
> mdio_bus.
> So I modified my net device driver so that in probe it calls
> of_mdiobus_register instead of mdiobus_register.
> And of_mdiobus_register seems to be looking for some kind of phy
> definitions in the device tree, which it does not find. And so it
> does not appear to register the bus in such a way that dsa.c can
> connect to it.
Hi Bryan
Are the sources for the ethernet driver available? I don't see them in
net-next.
There are two common ways for this to work, depending on the driver
architecture. Marvell devices have a separate mdio driver. In
kirkwood.dtsi you see:
mdio: mdio-bus@72004 {
compatible = "marvell,orion-mdio";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x72004 0x84>;
interrupts = <46>;
clocks = <&gate_clk 0>;
status = "disabled";
/* add phy nodes in board file */
};
and mvmdio.c calls of_mdiobus_register() passing this device node.
The other way is that the mdio is part of the ethernet
driver. e.g. for the Freescale FEC:
&fec1 {
phy-mode = "rmii";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_fec1>;
status = "okay";
mdio0: mdio {
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
};
};
In this case, of_mdiobus_register() is passed the mdio0 device node.
> &gpmc {
> status = "okay";
> ranges = <0 0 0x10000000 0x08000000>; // CS0: 128M
> pinctrl-names = "default";
> pinctrl-0 = <&gpmc_pins>;
> lan9352: ethernet@gpmc {
> compatible = "microchip,lan9352";
> interrupt-parent = <&gpio0>;
> interrupts = <7 8>;//7==GPIO bit 7, 8 = Active low level triggered.
>
> bank-width = <2>;
>
> phy-mode = "mii";
>
> reg = <0 0 0x10000>;
>
> reg-io-width = <4>;
> microchip,save-mac-address;
> microchip,irq-push-pull;
So i expect to see something like this here:
mdio0: mdio {
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
};
> };
> };
>
> / {
> dsa@0 {
> compatible = "microchip,dsa";
> #address-cells = <2>;
> #size-cells = <0>;
> dsa,ethernet = <&lan9352>;
> dsa,mii-bus = <&lan9352>;
and this would be
dsa,mii-bus = <&mdio0>;
> switch@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0 0>; /* MDIO address 0, switch 0 in tree */
> port@0 {
> reg = <0>;
> label = "cpu";
> };
> port@1 {
> reg = <1>;
> label = "lan1";
> };
> port@2 {
> reg = <2>;
> label = "lan2";
> };
> };
> };
> };
Andrew
^ permalink raw reply
* veth regression with "don’t modify ip_summed; doing so treats packets with bad checksums as good."
From: Ben Greear @ 2016-03-24 22:01 UTC (permalink / raw)
To: netdev, ej, vijayp; +Cc: Cong Wang
I have an application that creates two pairs of veth devices.
a <-> b c <-> d
b and c have a raw packet socket opened on them and I 'bridge' frames
between b and c to provide network emulation (ie, configurable delay).
I put IP 1.1.1.1/24 on a, 1.1.1.2/24 on d, and then create a UDP connection
(using policy based routing to ensure frames are sent on the appropriate
interfaces).
This is user-space only app, and kernel in this case is completely unmodified.
The commit below breaks this feature: UDP frames are sniffed on both a and d ports
(in both directions), but the UDP socket does not receive frames.
Using normal ethernet ports, this network emulation feature works fine, so it is
specific to VETH.
A similar test with just sending UDP between a single veth pair: e <-> f
works fine. Maybe it has something to do with raw packets?
The patch below is the culprit:
[greearb@ben-dt3 linux-2.6]$ git bisect bad
ce8c839b74e3017996fad4e1b7ba2e2625ede82f is the first bad commit
commit ce8c839b74e3017996fad4e1b7ba2e2625ede82f
Author: Vijay Pandurangan <vijayp@vijayp.ca>
Date: Fri Dec 18 14:34:59 2015 -0500
veth: don’t modify ip_summed; doing so treats packets with bad checksums as good.
Packets that arrive from real hardware devices have ip_summed ==
CHECKSUM_UNNECESSARY if the hardware verified the checksums, or
CHECKSUM_NONE if the packet is bad or it was unable to verify it. The
current version of veth will replace CHECKSUM_NONE with
CHECKSUM_UNNECESSARY, which causes corrupt packets routed from hardware to
a veth device to be delivered to the application. This caused applications
at Twitter to receive corrupt data when network hardware was corrupting
packets.
...
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0ef4a5a..ba21d07 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -117,12 +117,6 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
kfree_skb(skb);
goto drop;
}
- /* don't change ip_summed == CHECKSUM_PARTIAL, as that
- * will cause bad checksum on forwarded packets
- */
- if (skb->ip_summed == CHECKSUM_NONE &&
- rcv->features & NETIF_F_RXCSUM)
- skb->ip_summed = CHECKSUM_UNNECESSARY;
if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
struct pcpu_vstats *stats = this_cpu_ptr(dev->vstats);
Any suggestions for how to fix this so that I get the old working behaviour and
the bug this patch was trying to fix is also resolved?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply related
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Alexander Duyck @ 2016-03-24 21:50 UTC (permalink / raw)
To: Edward Cree
Cc: Or Gerlitz, Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <56F44B56.9070206@solarflare.com>
On Thu, Mar 24, 2016 at 1:17 PM, Edward Cree <ecree@solarflare.com> wrote:
> On 24/03/16 18:43, Alexander Duyck wrote:
>> On Thu, Mar 24, 2016 at 10:12 AM, Edward Cree <ecree@solarflare.com> wrote:
>>> For UDP header, we look to see if the current checksum field is zero. If
>>> so, we leave it as zero, fold our edits into csum_edit and return the
>>> result. Otherwise, we fold our edits and csum_edit into our checksum
>>> field, and return zero.
>> This would require changing how we handle partial checksums so that in
>> the case of UDP we don't allow 0 as a valid value. Currently we do.
>> It isn't till we get to the final checksum that we take care of the
>> bit flip in the case of 0.
> No, the UDP checksum will have been filled in by LCO and thus have been bit-
> flipped already if it was zero. Only the innermost L4 header will have a
> partial checksum, and that's TCP so the checksum is required. (Alternatively:
> whichever header has the partial checksum - and there is at most one - is
> identified by skb->csum_start, and by definition the checksum must be enabled
> for that header, so we can skip the 'check for zero' heuristic there.)
No, recheck the code. If the skb is GSO when we call udp_set_csum()
we only have populated the partial checksum in the UDP header. It
isn't until we actually perform the segmentation that we call
lco_csum().
> (Besides, I thought it was impossible for the partial checksum to be zero
> anyway because at least one of the inputs must be nonzero, and the end-
> around carry can never produce a zero. But maybe I'm getting confused here.)
I forgot about that bit. I think you are right. We end up inverting
the output from csum fold so we are back to 0x1 - 0xFFFF as possible
values.
>>> This should even be a worthwhile simplification of the non-nested case,
>>> because (if I understand correctly) it means GSO partial doesn't need the
>>> various gso_type flags we already have to specify tunnel type and checksum
>>> status; it just figures it out as it goes.
>> Yes, but doing packet inspection can get to be expensive as we crawl
>> through the headers. In addition it gets into the whole software
>> versus hardware offloads thing.
> The headers should already be in cache, I thought, and this is only happening
> once per superframe. We're already going to have to crawl through the headers
> anyway to edit the lengths, I don't think it should cost much more to also
> inspect things like GRE csum bit or the UDP checksum field. And by
> identifying the 'next header' from this method as well, we don't need to add a
> new SKB_GSO_FOO bit or two every time we add another kind of encapsulation to
> the kernel.
Right, but this gets back into the hardware flags issue. The whole
reason for SKB_GSO_FOO and SKB_GSO_FOO_CSUM is because hardware
typically supports one but not the other.
The other thing to keep in mind is it is much easier to figure out
what offloads we can make use of when we are only dealing with at most
1 layer of tunnels. When we start getting into stacked tunnels it
really becomes impossible to figure out what features in the hardware
we can still make use of. You throw everything and the kitchen sink
on it and we have to give up and would be doing everything in
software. At least if we restrict the features being requested the
hardware has a chance to perhaps be useful.
> (And remember that this is separate from - and doesn't impact - the existing
> GSO code; this is a bunch of new foo_gso_partial() callbacks, distinct from
> the foo_gso_segment() ones; and it's about preparing a superframe for hardware
> offload rather than doing the segmentation in software. I think it's
> preferable to have some preparation happen in software if that allows hardware
> to be totally dumb. (Apologies if that was already all obvious.))
This is where you and I differ. I think there are things you are overlooking.
For example one of the reasons why I know the outer UDP checksum works
the way it does with tunnel GSO is because the i40e and i40evf drivers
already have hardware that supports UDP_TUNNEL_CSUM. As such in order
to do what you are proposing we would likely have to rip that code out
and completely rewrite it since it would change out the checksum value
given to the device.
>> Honestly I think tunnel-in-tunnel is not going to be doable due to the
>> fact that we would have to increment multiple layers of IP ID in order
>> to do it correctly. The more I look into the whole DF on outer
>> headers thing the more I see RFCs such as RFC 2784 that say not to do
>> it unless you want to implement PMTU discovery, and PMTU discovery is
>> inherently flawed since it would require ICMP messages to be passed
>> which may be filtered out by firewalls.
> If PMTU discovery really is "inherently flawed", then TCP is broken and so
> is IPv6. I think the Right Thing here is for us to translate and forward
> ICMP errors to the originator of the traffic, as RFC 2784 vaguely suggests.
> It should also be possible to configure the tunnel endpoint's MTU to a
> sufficiently low value that in practice it will fit within the path MTU;
> then the sender will discover the tunnel's MTU restriction and stay within
> that. (I am assuming here that ICMP won't be filtered on the overlay
> network - which should be under the control of the tunnel administrator -
> even if it might be on the underlay network.)
I'm not going to get into an argument over the merits of PMTU. The
fact is I don't believe it is worth the effort in order to enable the
tunnel-in-tunnel case you envision.
Also another thing to keep in mind is you would have to find a way to
identify that a tunnel requesting to be segmented is setting the DF
bit in the outer headers.
>> On top of that it occurred to me that GRE cannot be screened in GRO
>> for the outer-IP-ID check. Basically what can happen is on devices
>> that don't parse inner headers for GRE we can end up with two TCP
>> flows from the same tunnel essentially stomping on each other and
>> causing one another to get evicted for having an outer IP-ID that
>> doesn't match up with the expected value.
> Yes, that does seem problematic - you'd need to save away the IPID check
> result and only evict once you'd ascertained they were the same flow. All
> the more reason to try to make your tunnels use DF *grin*.
Feel free to solve the problem if you believe it is that easy. There
is nothing in my patches to prevent that.
> On the subject of GRO, I was wondering - it seems like the main reason why
> drivers have LRO is so they can be more permissive than GRO's "must be
> reversible" rules. (At least, that seems to be the case for sfc's LRO,
> which is only in our out-of-tree driver.) Maybe instead of each driver
> having its own LRO code (with hacks to avoid breaking Slow Start and
> suchlike by breaking ACK stats), there should be per-device options to
> control how permissive GRO is (e.g. don't check IP IDs), so that a user who
> wants LRO-like behaviour can get it from GRO.
I don't really see the point. Odds are the LRO in the driver isn't
too much more efficient than the in-kernel GRO. If anything I would
think the main motivation for maintaining LRO in your out-of-tree
driver is to support kernels prior to GRO.
> Any obvious reason why I couldn't do such a thing?
>
> (I realise LRO might not go away entirely, if other drivers are doing
> various hardware-assisted things. But in our case it really is all in
> software.)
I don't know. I'm thinking it all depends on what kind of stuff you
are talking about trying to change. Most of the flush conditions in
GRO have very good reasons for being there. I know in the case of the
Intel drivers I actually found that by copying most of the conditions
into the out-of-tree driver LRO code we had I saw improvements. More
often then not the more aggressive LRO can end up causing more harm
than good because what ends up happening is that it spends too much
time aggregating and starves the other side by witholding acks.
- Alex
^ permalink raw reply
* Re: [RFT Patch net 1/2] ipv6: invalidate the socket cached route on pmtu events if possible
From: Wei Wang @ 2016-03-24 21:38 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Wei Wang, Steffen Klassert,
Martin KaFai Lau, Hannes Frederic Sowa, Julian Anastasov,
Eric Dumazet
In-Reply-To: <1458851740.12033.24.camel@edumazet-glaptop3.roam.corp.google.com>
> + if (new)
> + ip6_dst_store(sk, dst, NULL, NULL);
> +
I read through the code for ip6_dst_store(), the last 2 params are for
sk->daddr_cache and sk->saddr_cache.
Those are examined and used in ip6_sk_dst_lookup_flow(). If those 2
(mostly only check sk->daddr_cache) are valid, it will try to use
sk->sk_dst_cache and avoid ip6_route_output() call. If we put it as
NULL, it is guaranteed to not pass the validation check and do
ip6_route_output() lookup.
So we probably don't want to use NULL but something similar as used in
udpv6_sendmsg() like the following?
ip6_dst_store(sk, dst,
ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
&sk->sk_v6_daddr : NULL,
#ifdef CONFIG_IPV6_SUBTREES
ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
&np->saddr :
#endif
NULL);
Thanks.
Wei
^ permalink raw reply
* RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver
From: Bryan.Whitehead @ 2016-03-24 21:16 UTC (permalink / raw)
To: andrew; +Cc: davem, netdev
In-Reply-To: <20160219201415.GA9884@lunn.ch>
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, February 19, 2016 3:14 PM
> > I'm not yet sure how it attaches to the underlying Ethernet driver.
>
> The DSA core does that for you. If you look at the device tree
> binding:
>
> dsa@0 {
> compatible = "marvell,dsa";
> #address-cells = <2>;
> #size-cells = <0>;
>
> interrupts = <10>;
> dsa,ethernet = <ðernet0>;
>
> So this says which Ethernet interface to use. net/dsa/dsa.c will create a slave
> interface per external port of your switch. This slave is a netdev. Frames
> transmitted by this slave are fed through the tag code to add additional
> headers/trailers, and then passed to this ethernet device. Frames received
> by the ethernet again through the tag code, stripping off any headers/trails
> and demuxing to the correct slave.
Hi Andrew,
Sorry to bother you with this. But I'm having major difficulty with getting my dsa driver entry points called.
Here is what I've done so far.
I copied drivers/net/dsa/mv88x6060.c into drivers/net/dsa/mchp9352_dsa.c
I then modified mchp9352_dsa.c as follows
I emptied out the function bodies, and replaced them with a printk("Not Implemented\n");
I did the same thing with net/dsa/tag_trailer.c which was copied into net/dsa/tag_mchp9352.c
And function bodies were replaced with printk("Not Implemented\n");
I also modified net/dsa/dsa.c, and net/dsa/slave.c, to include the hooks into my new tag files.
My intent was to just see one of my "Not Implemented" functions called, and then I would focus on implementing it.
But so far I have not seen any of my "Not Implemented" functions called.
Here is what I know so far.
It appears the dsa.c is not able to attach my underlying net device. And that seems to be due to it is unable to find the mdio_bus.
So I modified my net device driver so that in probe it calls of_mdiobus_register instead of mdiobus_register.
And of_mdiobus_register seems to be looking for some kind of phy definitions in the device tree, which it does not find. And so it does not appear to register the bus in such a way that dsa.c can connect to it.
So the problem appears to be an issue with my device tree, which is partially shown below.
&gpmc {
status = "okay";
ranges = <0 0 0x10000000 0x08000000>; // CS0: 128M
pinctrl-names = "default";
pinctrl-0 = <&gpmc_pins>;
lan9352: ethernet@gpmc {
compatible = "microchip,lan9352";
interrupt-parent = <&gpio0>;
interrupts = <7 8>;//7==GPIO bit 7, 8 = Active low level triggered.
bank-width = <2>;
phy-mode = "mii";
reg = <0 0 0x10000>;
reg-io-width = <4>;
microchip,save-mac-address;
microchip,irq-push-pull;
};
};
/ {
dsa@0 {
compatible = "microchip,dsa";
#address-cells = <2>;
#size-cells = <0>;
dsa,ethernet = <&lan9352>;
dsa,mii-bus = <&lan9352>;
switch@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0 0>; /* MDIO address 0, switch 0 in tree */
port@0 {
reg = <0>;
label = "cpu";
};
port@1 {
reg = <1>;
label = "lan1";
};
port@2 {
reg = <2>;
label = "lan2";
};
};
};
};
I modeled this from what I found in
arch/arm/boot/dts/kirkwood-mv88f6281gtw-ge.dts
So my question is. Can you see anything that I'm doing wrong?
Given the issue seems to be about the lan9352 node not having child phy nodes, then I wonder if I'm not looking at the best example since my example does not use child phy nodes either. If so, can you point me to a better example?
Anyway I'm really hitting a road block here, so any and all guidance is greatly appreciated.
Thanks,
Bryan
^ permalink raw reply
* Re: [RFT Patch net 1/2] ipv6: invalidate the socket cached route on pmtu events if possible
From: Eric Dumazet @ 2016-03-24 20:35 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, weiwan, Steffen Klassert, Martin KaFai Lau,
Hannes Frederic Sowa, Julian Anastasov
In-Reply-To: <1458847536-7729-1-git-send-email-xiyou.wangcong@gmail.com>
On Thu, 2016-03-24 at 12:25 -0700, Cong Wang wrote:
> void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
> {
> - ip6_update_pmtu(skb, sock_net(sk), mtu,
> - sk->sk_bound_dev_if, sk->sk_mark);
> + const struct ipv6hdr *iph = (struct ipv6hdr *)skb->data;
> + struct net *net = sock_net(sk);
> + struct dst_entry *ndst, *dst;
> + struct flowi6 fl6;
> + bool new = false;
> +
> + memset(&fl6, 0, sizeof(fl6));
> +
> + bh_lock_sock(sk);
> +
This is not clear why you need to acquire socket lock.
sk_dst_cache is protected by an atomic operation.
udp for example calls ip6_dst_store() without socket being locked.
^ permalink raw reply
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Edward Cree @ 2016-03-24 20:17 UTC (permalink / raw)
To: Alexander Duyck
Cc: Or Gerlitz, Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <CAKgT0UeW7A6cMRHm8_nEX=op_VAK5wcoYYRTNQVmMQWe02HdfA@mail.gmail.com>
On 24/03/16 18:43, Alexander Duyck wrote:
> On Thu, Mar 24, 2016 at 10:12 AM, Edward Cree <ecree@solarflare.com> wrote:
>> For UDP header, we look to see if the current checksum field is zero. If
>> so, we leave it as zero, fold our edits into csum_edit and return the
>> result. Otherwise, we fold our edits and csum_edit into our checksum
>> field, and return zero.
> This would require changing how we handle partial checksums so that in
> the case of UDP we don't allow 0 as a valid value. Currently we do.
> It isn't till we get to the final checksum that we take care of the
> bit flip in the case of 0.
No, the UDP checksum will have been filled in by LCO and thus have been bit-
flipped already if it was zero. Only the innermost L4 header will have a
partial checksum, and that's TCP so the checksum is required. (Alternatively:
whichever header has the partial checksum - and there is at most one - is
identified by skb->csum_start, and by definition the checksum must be enabled
for that header, so we can skip the 'check for zero' heuristic there.)
(Besides, I thought it was impossible for the partial checksum to be zero
anyway because at least one of the inputs must be nonzero, and the end-
around carry can never produce a zero. But maybe I'm getting confused here.)
>> This should even be a worthwhile simplification of the non-nested case,
>> because (if I understand correctly) it means GSO partial doesn't need the
>> various gso_type flags we already have to specify tunnel type and checksum
>> status; it just figures it out as it goes.
> Yes, but doing packet inspection can get to be expensive as we crawl
> through the headers. In addition it gets into the whole software
> versus hardware offloads thing.
The headers should already be in cache, I thought, and this is only happening
once per superframe. We're already going to have to crawl through the headers
anyway to edit the lengths, I don't think it should cost much more to also
inspect things like GRE csum bit or the UDP checksum field. And by
identifying the 'next header' from this method as well, we don't need to add a
new SKB_GSO_FOO bit or two every time we add another kind of encapsulation to
the kernel.
(And remember that this is separate from - and doesn't impact - the existing
GSO code; this is a bunch of new foo_gso_partial() callbacks, distinct from
the foo_gso_segment() ones; and it's about preparing a superframe for hardware
offload rather than doing the segmentation in software. I think it's
preferable to have some preparation happen in software if that allows hardware
to be totally dumb. (Apologies if that was already all obvious.))
> Honestly I think tunnel-in-tunnel is not going to be doable due to the
> fact that we would have to increment multiple layers of IP ID in order
> to do it correctly. The more I look into the whole DF on outer
> headers thing the more I see RFCs such as RFC 2784 that say not to do
> it unless you want to implement PMTU discovery, and PMTU discovery is
> inherently flawed since it would require ICMP messages to be passed
> which may be filtered out by firewalls.
If PMTU discovery really is "inherently flawed", then TCP is broken and so
is IPv6. I think the Right Thing here is for us to translate and forward
ICMP errors to the originator of the traffic, as RFC 2784 vaguely suggests.
It should also be possible to configure the tunnel endpoint's MTU to a
sufficiently low value that in practice it will fit within the path MTU;
then the sender will discover the tunnel's MTU restriction and stay within
that. (I am assuming here that ICMP won't be filtered on the overlay
network - which should be under the control of the tunnel administrator -
even if it might be on the underlay network.)
> On top of that it occurred to me that GRE cannot be screened in GRO
> for the outer-IP-ID check. Basically what can happen is on devices
> that don't parse inner headers for GRE we can end up with two TCP
> flows from the same tunnel essentially stomping on each other and
> causing one another to get evicted for having an outer IP-ID that
> doesn't match up with the expected value.
Yes, that does seem problematic - you'd need to save away the IPID check
result and only evict once you'd ascertained they were the same flow. All
the more reason to try to make your tunnels use DF *grin*.
On the subject of GRO, I was wondering - it seems like the main reason why
drivers have LRO is so they can be more permissive than GRO's "must be
reversible" rules. (At least, that seems to be the case for sfc's LRO,
which is only in our out-of-tree driver.) Maybe instead of each driver
having its own LRO code (with hacks to avoid breaking Slow Start and
suchlike by breaking ACK stats), there should be per-device options to
control how permissive GRO is (e.g. don't check IP IDs), so that a user who
wants LRO-like behaviour can get it from GRO.
Any obvious reason why I couldn't do such a thing?
(I realise LRO might not go away entirely, if other drivers are doing
various hardware-assisted things. But in our case it really is all in
software.)
-Ed
^ permalink raw reply
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Martin KaFai Lau @ 2016-03-24 19:45 UTC (permalink / raw)
To: Wei Wang, Eric Dumazet
Cc: Eric Dumazet, Cong Wang, Wei Wang, David Miller,
Linux Kernel Network Developers
In-Reply-To: <20160324183228.GA95371@kafai-mba.local>
On Thu, Mar 24, 2016 at 11:32:28AM -0700, Martin KaFai Lau wrote:
> Some fast path may do __sk_dst_get() which does not refcnt the sk->sk_dst_cache.
Ignore this comment. This should have been protected by rcu grace period.
^ permalink raw reply
* Re: [RFT Patch net 2/2] ipv6: add a socket release callback for datagram sockets
From: Cong Wang @ 2016-03-24 19:40 UTC (permalink / raw)
To: Linux Kernel Network Developers
Cc: Wei Wang, Cong Wang, Steffen Klassert, Martin KaFai Lau,
Hannes Frederic Sowa, Julian Anastasov
In-Reply-To: <1458847536-7729-2-git-send-email-xiyou.wangcong@gmail.com>
On Thu, Mar 24, 2016 at 12:25 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> + dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
> + dst = !IS_ERR(rt) ? &rt->dst : NULL;
Oops, this one is clearly a copy-n-paste mistake...
^ permalink raw reply
* [RFT Patch net 2/2] ipv6: add a socket release callback for datagram sockets
From: Cong Wang @ 2016-03-24 19:25 UTC (permalink / raw)
To: netdev
Cc: weiwan, Cong Wang, Steffen Klassert, Martin KaFai Lau,
Hannes Frederic Sowa, Julian Anastasov
In-Reply-To: <1458847536-7729-1-git-send-email-xiyou.wangcong@gmail.com>
Similar to commit 8141ed9fcedb, this implements a socket
release callback function to check if an IPv6 socket cached
route got invalid during the time we owned the socket.
The function is used from udp, raw sockets.
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/ipv6.h | 1 +
net/ipv6/datagram.c | 40 ++++++++++++++++++++++++++++++++++++++++
net/ipv6/raw.c | 1 +
net/ipv6/udp.c | 1 +
4 files changed, 43 insertions(+)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index d0aeb97..890456d 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -959,6 +959,7 @@ int compat_ipv6_getsockopt(struct sock *sk, int level, int optname,
int ip6_datagram_connect(struct sock *sk, struct sockaddr *addr, int addr_len);
int ip6_datagram_connect_v6_only(struct sock *sk, struct sockaddr *addr,
int addr_len);
+void ip6_datagram_release_cb(struct sock *sk);
int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len,
int *addr_len);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 4281621..a743caa 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -231,6 +231,46 @@ int ip6_datagram_connect_v6_only(struct sock *sk, struct sockaddr *uaddr,
}
EXPORT_SYMBOL_GPL(ip6_datagram_connect_v6_only);
+void ip6_datagram_release_cb(struct sock *sk)
+{
+ const struct inet_sock *inet = inet_sk(sk);
+ struct ipv6_pinfo *np = inet6_sk(sk);
+ struct in6_addr *final_p, final;
+ struct ipv6_txoptions *opt;
+ struct dst_entry *dst;
+ struct flowi6 fl6;
+ struct rtable *rt;
+
+ rcu_read_lock();
+
+ dst = __sk_dst_get(sk);
+ if (!dst || !dst->obsolete || dst->ops->check(dst, 0)) {
+ rcu_read_unlock();
+ return;
+ }
+
+ memset(&fl6, 0, sizeof(fl6));
+ fl6.flowi6_proto = sk->sk_protocol;
+ fl6.daddr = sk->sk_v6_daddr;
+ fl6.saddr = np->saddr;
+ fl6.flowi6_oif = sk->sk_bound_dev_if;
+ fl6.flowi6_mark = sk->sk_mark;
+ fl6.fl6_dport = inet->inet_dport;
+ fl6.fl6_sport = inet->inet_sport;
+
+ rcu_read_lock();
+ opt = rcu_dereference(np->opt);
+ final_p = fl6_update_dst(&fl6, opt, &final);
+ rcu_read_unlock();
+
+ dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
+ dst = !IS_ERR(rt) ? &rt->dst : NULL;
+ sk_dst_set(sk, dst);
+
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(ip6_datagram_release_cb);
+
void ipv6_icmp_error(struct sock *sk, struct sk_buff *skb, int err,
__be16 port, u32 info, u8 *payload)
{
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index fa59dd7..4319e65 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -1235,6 +1235,7 @@ struct proto rawv6_prot = {
.recvmsg = rawv6_recvmsg,
.bind = rawv6_bind,
.backlog_rcv = rawv6_rcv_skb,
+ .release_cb = ip6_datagram_release_cb,
.hash = raw_hash_sk,
.unhash = raw_unhash_sk,
.obj_size = sizeof(struct raw6_sock),
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index fd25e44..0fdaf8f 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1539,6 +1539,7 @@ struct proto udpv6_prot = {
.sendmsg = udpv6_sendmsg,
.recvmsg = udpv6_recvmsg,
.backlog_rcv = __udpv6_queue_rcv_skb,
+ .release_cb = ip6_datagram_release_cb,
.hash = udp_lib_hash,
.unhash = udp_lib_unhash,
.rehash = udp_v6_rehash,
--
2.1.0
^ permalink raw reply related
* [RFT Patch net 1/2] ipv6: invalidate the socket cached route on pmtu events if possible
From: Cong Wang @ 2016-03-24 19:25 UTC (permalink / raw)
To: netdev
Cc: weiwan, Cong Wang, Steffen Klassert, Martin KaFai Lau,
Hannes Frederic Sowa, Julian Anastasov
Similar to commit 9cb3a50c5f63, with this patch we invalidate the
socket cached route if possible. If the socket is owened by the
user, we can't update the cached route directly.
Reported-by: Wei Wang <weiwan@google.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv6/route.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 65 insertions(+), 9 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed44663..2c16cbc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1346,18 +1346,20 @@ static bool rt6_cache_allowed_for_pmtu(const struct rt6_info *rt)
(rt->rt6i_flags & RTF_PCPU || rt->rt6i_node);
}
-static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
- const struct ipv6hdr *iph, u32 mtu)
+static struct dst_entry *__ip6_rt_update_pmtu(struct dst_entry *dst,
+ const struct sock *sk,
+ const struct ipv6hdr *iph,
+ u32 mtu, bool hold)
{
struct rt6_info *rt6 = (struct rt6_info *)dst;
if (rt6->rt6i_flags & RTF_LOCAL)
- return;
+ return dst;
dst_confirm(dst);
mtu = max_t(u32, mtu, IPV6_MIN_MTU);
if (mtu >= dst_mtu(dst))
- return;
+ return dst;
if (!rt6_cache_allowed_for_pmtu(rt6)) {
rt6_do_update_pmtu(rt6, mtu);
@@ -1372,11 +1374,13 @@ static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
daddr = &sk->sk_v6_daddr;
saddr = &inet6_sk(sk)->saddr;
} else {
- return;
+ return dst;
}
nrt6 = ip6_rt_cache_alloc(rt6, daddr, saddr);
if (nrt6) {
rt6_do_update_pmtu(nrt6, mtu);
+ if (hold)
+ dst_hold(&nrt6->dst);
/* ip6_ins_rt(nrt6) will bump the
* rt6->rt6i_node->fn_sernum
@@ -1384,14 +1388,17 @@ static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
* invalidate the sk->sk_dst_cache.
*/
ip6_ins_rt(nrt6);
+ return &nrt6->dst;
}
}
+
+ return dst;
}
static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
struct sk_buff *skb, u32 mtu)
{
- __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
+ __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu, false);
}
void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
@@ -1410,15 +1417,64 @@ void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
dst = ip6_route_output(net, NULL, &fl6);
if (!dst->error)
- __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
+ __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu), false);
dst_release(dst);
}
EXPORT_SYMBOL_GPL(ip6_update_pmtu);
void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
{
- ip6_update_pmtu(skb, sock_net(sk), mtu,
- sk->sk_bound_dev_if, sk->sk_mark);
+ const struct ipv6hdr *iph = (struct ipv6hdr *)skb->data;
+ struct net *net = sock_net(sk);
+ struct dst_entry *ndst, *dst;
+ struct flowi6 fl6;
+ bool new = false;
+
+ memset(&fl6, 0, sizeof(fl6));
+
+ bh_lock_sock(sk);
+
+ fl6.flowi6_oif = sk->sk_bound_dev_if;
+ fl6.flowi6_mark = sk->sk_mark ? : IP6_REPLY_MARK(net, skb->mark);
+ fl6.daddr = iph->daddr;
+ fl6.saddr = iph->saddr;
+ fl6.flowlabel = ip6_flowinfo(iph);
+
+ dst = sk_dst_get(sk);
+ if (sock_owned_by_user(sk) || !dst) {
+ ip6_update_pmtu(skb, net, mtu, fl6.flowi6_oif, fl6.flowi6_mark);
+ goto out;
+ }
+
+ if (!dst_check(dst, 0)) {
+ dst_release(dst);
+ dst = ip6_route_output(net, sk, &fl6);
+ if (dst->error)
+ goto out;
+
+ new = true;
+ }
+
+ ndst = __ip6_rt_update_pmtu(dst->path, sk, iph, ntohl(mtu), true);
+ if (!dst_check(ndst, 0)) {
+ if (ndst != dst)
+ dst_release(dst);
+
+ dst = ip6_route_output(net, sk, &fl6);
+ if (dst->error)
+ goto out;
+
+ new = true;
+ } else if (ndst != dst) {
+ new = true;
+ }
+
+ if (new)
+ ip6_dst_store(sk, dst, NULL, NULL);
+
+out:
+ bh_unlock_sock(sk);
+ dst_release(dst);
}
EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Cong Wang @ 2016-03-24 19:23 UTC (permalink / raw)
To: Eric Dumazet
Cc: Wei Wang, Martin KaFai Lau, Wei Wang, David Miller, Eric Dumazet,
Linux Kernel Network Developers
In-Reply-To: <1458689814.10868.29.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Mar 22, 2016 at 4:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-03-22 at 13:13 -0700, Cong Wang wrote:
>> On Tue, Mar 22, 2016 at 11:03 AM, Wei Wang <tracywwnj@gmail.com> wrote:
>> > Thanks Martin and Cong.
>> >
>> > I guess then we are going with the following fix in ip6_sk_update_pmtu():
>> > 1. call ip6_upate_pmtu() as it is
>> > 2. do a dst_check()
>> > 3. re-lookup() if it is invalid
>> > 4. and then do a ip6_dst_store()/dst_set
>>
>> Exactly, please try the attached patch. Note I did nothing more than a
>> compile test.
>>
>> Does it make sense to you now?
>
>
> Hard to reply on your patch as it was not inlined.
Sorry about that, it is still not ready to send out formally yet, because
I don't do any run-time tests yet.
>
> 1) Lot of code duplication, for some reason I do not yet understand.
>
> ip6_sk_update_pmtu() and ip6_update_pmtu() will basically do the same
> thing...
Hmm? After my patch ip6_sk_update_pmtu() will take care of sk_dst_cache,
but ip6_update_pmtu() doesn't do that since it has no 'sk' parameter.
It is pretty much similar to commit 9cb3a50c5f63.
>
> 2)
>
> + if (sk->sk_state == TCP_ESTABLISHED)
> + ip6_dst_store(sk, dst, &iph->daddr, &iph->saddr);
> +out:
>
>
> ip6_dst_store() will do :
>
> np->daddr_cache = daddr; (&iph->daddr)
> np->saddr_cache = saddr; (&iph->saddr)
>
> So when skb is freed, daddr_cache & saddr_cache point to freed data.
Ah, good catch, so ip6_dst_store() can only use the addr's stored
in socket or NULL.
I will send out 2 formal patches shortly.
^ permalink raw reply
* Re: [PATCH net v2 2/2] net: bcmgenet: fix skb_len in bcmgenet_xmit_single()
From: David Miller @ 2016-03-24 19:15 UTC (permalink / raw)
To: pgynther; +Cc: netdev, f.fainelli, jaedon.shin, edumazet
In-Reply-To: <1458844041-71532-2-git-send-email-pgynther@google.com>
From: Petri Gynther <pgynther@google.com>
Date: Thu, 24 Mar 2016 11:27:21 -0700
> skb_len needs to be skb_headlen(skb) in bcmgenet_xmit_single().
>
> Fragmented skbs can have only Ethernet + IP + TCP headers (14+20+20=54 bytes)
> in the linear buffer, followed by the rest in fragments. Bumping skb_len to
> ETH_ZLEN would be incorrect for this case, as it would introduce garbage
> between TCP header and the fragment data.
>
> This also works with regular/non-fragmented small packets < ETH_ZLEN bytes.
> Successfully tested this on GENETv3 with 42-byte ARP frames.
>
> For testing, I used:
> ethtool -K eth0 tx-checksum-ipv4 off
> ethtool -K eth0 tx-checksum-ipv6 off
> echo 0 > /proc/sys/net/ipv4/tcp_timestamps
>
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Petri Gynther <pgynther@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH net v2 1/2] net: bcmgenet: fix dev->stats.tx_bytes accounting
From: David Miller @ 2016-03-24 19:15 UTC (permalink / raw)
To: pgynther; +Cc: netdev, f.fainelli, jaedon.shin, edumazet
In-Reply-To: <1458844041-71532-1-git-send-email-pgynther@google.com>
From: Petri Gynther <pgynther@google.com>
Date: Thu, 24 Mar 2016 11:27:20 -0700
> 1. Add bytes_compl local variable to __bcmgenet_tx_reclaim() to collect
> transmitted bytes. dev->stats updates can then be moved outside the
> while-loop. bytes_compl is also needed for future BQL support.
> 2. When bcmgenet device uses Tx checksum offload, each transmitted skb
> gets an extra 64-byte header prepended to it. Before this header is
> prepended to the skb, we need to save the skb "wire" length in
> GENET_CB(skb)->bytes_sent, so that proper Tx bytes accounting can
> be done in __bcmgenet_tx_reclaim().
> 3. skb->len covers the entire length of skb, whether it is linear or
> fragmented. Thus, when we clean the fragments, do not increase
> transmitted bytes.
>
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Petri Gynther <pgynther@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH] netpoll: Fix extra refcount release in netpoll_cleanup()
From: David Miller @ 2016-03-24 18:57 UTC (permalink / raw)
To: bhelgaas; +Cc: nikolay, netdev, nhorman, aduyck, linux-kernel
In-Reply-To: <20160324161334.31279.26510.stgit@bhelgaas-glaptop2.roam.corp.google.com>
From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 24 Mar 2016 11:13:34 -0500
> netpoll_setup() does a dev_hold() on np->dev, the netpoll device. If it
> fails, it correctly does a dev_put() but leaves np->dev set. If we call
> netpoll_cleanup() after the failure, np->dev is still set so we do another
> dev_put(), which decrements the refcount an extra time.
>
> It's questionable to call netpoll_cleanup() after netpoll_setup() fails,
> but it can be difficult to find the problem, and we can easily avoid it in
> this case. The extra decrements can lead to hangs like this:
>
> unregister_netdevice: waiting for bond0 to become free. Usage count = -3
>
> In __netpoll_setup(), don't set np->dev until we know we're going to
> succeed.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
The reason this bug exists is because the thing doing the reference
counting is separated from the thing that assigns the device pointer.
That's how this was allowed to happen.
If you instead do the np->dev = dev; assignment where the get is
performed, and do a np->dev = NULL; where the error path puts
the reference, everything is obvious and this error is unlikely to
be reintroduced.
So could you please implement your fix like that?
Thanks.
^ permalink raw reply
* Re: [PATCH net] switchdev: fix typo in comments/doc
From: David Miller @ 2016-03-24 18:51 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev
In-Reply-To: <1458834600-5395-1-git-send-email-nicolas.dichtel@6wind.com>
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 24 Mar 2016 16:50:00 +0100
> Two minor typo.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH 1/1] net: macb: replace macb_writel() call by queue_writel() to update queue ISR
From: David Miller @ 2016-03-24 18:50 UTC (permalink / raw)
To: cyrille.pitchen
Cc: nicolas.ferre, linux-arm-kernel, netdev, soren.brinkmann,
linux-kernel
In-Reply-To: <1458830404-6213-1-git-send-email-cyrille.pitchen@atmel.com>
From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Date: Thu, 24 Mar 2016 15:40:04 +0100
> macb_interrupt() should not use macb_writel(bp, ISR, <value>) but only
> queue_writel(queue, ISR, <value>).
>
> There is one IRQ and one set of {ISR, IER, IDR, IMR} [1] registers per
> queue on gem hardware, though only queue0 is actually used for now to
> receive frames: other queues can already be used to transmit frames.
>
> The queue_readl() and queue_writel() helper macros are designed to access
> the relevant IRQ registers.
>
> [1]
> ISR: Interrupt Status Register
> IER: Interrupt Enable Register
> IDR: Interrupt Disable Register
> IMR: Interrupt Mask Register
>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Fixes: bfbb92c44670 ("net: macb: Handle the RXUBR interrupt on all devices")
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem
From: Andrew Lunn @ 2016-03-24 18:50 UTC (permalink / raw)
To: Vishal Thanki; +Cc: Florian Fainelli, Matus Ujhelyi, netdev
In-Reply-To: <CAC3a_SDK8aoz1KKXa9FYBj8a1q8EPMLtVe94mGemeh7Wf=BL4g@mail.gmail.com>
> Hi Andrew,
>
> I still have some questions. Will the phylib call this
> led_trigger_register_to_led() function for registering the trigger
> instead of calling led_trigger_register()?
Each phy driver needs to call led_classdev_register() for each LED it
has. It then also calls led_trigger_register_to_led() with the
triggers that each LED supports.
> > function which registers a trigger to a specific LED, on its own
> > trigger list. led_trigger_store() and led_trigger_show() would use
> > both lists.
>
> In case of led_trigger_store(), how to stop the non-PHY LEDs to
> register themselves from eth-phy-activity trigger.
They won't have the option. led_trigger_register_to_led() only
registers the trigger to one specific LED, i.e. adds it to the trigger
list in its led_classdev structure, not the global list of triggers.
Andrew
^ permalink raw reply
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Alexander Duyck @ 2016-03-24 18:43 UTC (permalink / raw)
To: Edward Cree
Cc: Or Gerlitz, Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <56F41FFF.7000508@solarflare.com>
On Thu, Mar 24, 2016 at 10:12 AM, Edward Cree <ecree@solarflare.com> wrote:
> On 23/03/16 23:15, Alexander Duyck wrote:
>> Right, but the problem becomes how do you identify what tunnel wants
>> what. So for example we could theoretically have a UDP tunnel in a
>> UDP with checksum. How would we tell which one want to have the
>> checksum set and which one doesn't? The fact is we cannot.
> I think we can still handle that, assuming the device is only touching the
> innermost checksum (i.e. it's obeying csum_start/offset). We don't need
> flags to tell us what to fill in in GSO, we can work it all out:
> Make the series of per-protocol callbacks for GSO partial run inner-
> outwards, by using recursion at the head. Make each return a csum_edit
> value. Then for example:
> For IPv4 header, our checksum covers only our header, so we fold any edits
> into our own checksum, and pass csum_edit through unchanged.
Right. IPv4 is easy because it is a localized checksum that is always present.
> For UDP header, we look to see if the current checksum field is zero. If
> so, we leave it as zero, fold our edits into csum_edit and return the
> result. Otherwise, we fold our edits and csum_edit into our checksum
> field, and return zero.
This would require changing how we handle partial checksums so that in
the case of UDP we don't allow 0 as a valid value. Currently we do.
It isn't till we get to the final checksum that we take care of the
bit flip in the case of 0.
> For GRE, we look at the checksumming bit in the GRE header, and behave
> similarly to UDP.
> Etcetera...
Right. In the case of GRE we at least have a flag we could check.
> This should even be a worthwhile simplification of the non-nested case,
> because (if I understand correctly) it means GSO partial doesn't need the
> various gso_type flags we already have to specify tunnel type and checksum
> status; it just figures it out as it goes.
Yes, but doing packet inspection can get to be expensive as we crawl
through the headers. In addition it gets into the whole software
versus hardware offloads thing.
> If your device is touching other checksums as well, then of course you need
> to figure that out in your driver so you can cancel it out. But the device
> will only fiddle with the headers you tell it about (in your case I think
> that's outermost L3), not any others in the middle. So it should still all
> work, without the driver having to know about the nesting.
>
>> You are
>> looking too far ahead. We haven't gotten to tunnel in tunnel yet.
> IMHO, if our offloads are truly generic, tunnel in tunnel should be low-
> hanging fruit. (In principle, "VxLAN + Ethernet + IP + GRE" is just
> another encapsulation header, albeit a rather long one). Therefore, if
> it _isn't_ low-hanging fruit for us, we should suspect that we aren't
> generic. So even if it's not currently useful in itself, it's still a
> convenient canary.
Honestly I think tunnel-in-tunnel is not going to be doable due to the
fact that we would have to increment multiple layers of IP ID in order
to do it correctly. The more I look into the whole DF on outer
headers thing the more I see RFCs such as RFC 2784 that say not to do
it unless you want to implement PMTU discovery, and PMTU discovery is
inherently flawed since it would require ICMP messages to be passed
which may be filtered out by firewalls.
On top of that it occurred to me that GRE cannot be screened in GRO
for the outer-IP-ID check. Basically what can happen is on devices
that don't parse inner headers for GRE we can end up with two TCP
flows from the same tunnel essentially stomping on each other and
causing one another to get evicted for having an outer IP-ID that
doesn't match up with the expected value.
- Alex
^ permalink raw reply
* Re: [PATCH 1/1] net: Add SO_REUSEPORT_LISTEN_OFF socket option as drain mode
From: Eric Dumazet @ 2016-03-24 18:37 UTC (permalink / raw)
To: Tolga Ceylan
Cc: Daniel Borkmann, Tom Herbert, Willy Tarreau, Craig Gallek,
Josh Snyder, Aaron Conole, David S. Miller,
Linux Kernel Network Developers
In-Reply-To: <CALmu+SxF5dDEBwpk-R=V5DGZBed1rvjYGEJ+cU8ZhyAdLWshug@mail.gmail.com>
On Thu, 2016-03-24 at 11:20 -0700, Tolga Ceylan wrote:
> On Thu, Mar 24, 2016 at 10:55 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On 03/24/2016 06:26 PM, Tom Herbert wrote:
> >>
> >> I completely agree with this, but I wonder if we now need a repository
> >> of useful BPF modules. So in the case of implementing functionality
> >> like in SO_REUSEPORT_LISTEN_OFF that might just become a common BPF
> >> program we could direct people to use.
> >
> >
> > Good point. There's tools/testing/selftests/net/ containing already
> > reuseport
> > BPF example, maybe it could be extended.
>
> I would appreciate a conceptual description on how this would work
> especially for a common scenario
> as described by Willy. My initial impression was that a coordinator
> (master) process takes this
> responsibility to adjust BPF filters as children come and go.
>
> Two popular software has similar use cases: nginx and haproxy. Another
> concern is with the
> introduction of BPF itself, should we expect a performance drop in
> these applications?
Just to make it clear :
BPF allows proper siloing if you have multi queue NIC, instead of a
random hashing that was reducing performance.
BPF on SO_REUSEPORT can reduce false sharing between cpus and increase
NUMA locality.
BPF allows us to use whatever number of silos, without having to scan
the whole socket list for every incoming packet.
Huge gain really.
^ permalink raw reply
* Re: [PATCH net 0/7] net: hns: fix some bugs in HNS driver
From: David Miller @ 2016-03-24 18:33 UTC (permalink / raw)
To: Yisen.Zhuang
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, xieqianqian, netdev, linux-kernel, linuxarm
In-Reply-To: <1458817686-159708-1-git-send-email-Yisen.Zhuang@huawei.com>
From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
Date: Thu, 24 Mar 2016 19:07:59 +0800
> Here are some bug fixed patches for HNS driver.
>
> They are:
>
>>from Kejian, fix for the warning of passing zero to 'PTR_ERR'
>
>>from qianqian, four fixes for inappropriate operation in hns driver
>
>>from Sheng, one fix for optimization of irq proccess in hns driver, and
> one fix for hilink status for hns driver.
>
> For more details, please see individual patches.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Martin KaFai Lau @ 2016-03-24 18:32 UTC (permalink / raw)
To: Wei Wang, Eric Dumazet
Cc: Eric Dumazet, Cong Wang, Wei Wang, David Miller,
Linux Kernel Network Developers
In-Reply-To: <CAC15z3g-LDOdpcsZEs6XFFOOb92bXAbdOORdnd+MTvF62TgSiA@mail.gmail.com>
On Wed, Mar 23, 2016 at 04:57:22PM -0700, Wei Wang wrote:
> What about something like this:
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index ed44663..21b4102 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1394,6 +1394,19 @@ static void ip6_rt_update_pmtu(struct dst_entry
> *dst, struct sock *sk,
> __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
> }
>
> +static void ip6_fill_in_flow(struct flowi6 *fl6, struct net *net,
> + struct sk_buff *skb, int oif, u32 mark)
> +{
> + const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
> +
> + memset(fl6, 0, sizeof(fl6));
> + fl6->flowi6_oif = oif;
> + fl6->flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
> + fl6->daddr = iph->daddr;
> + fl6->saddr = iph->saddr;
> + fl6->flowlabel = ip6_flowinfo(iph);
> +}
> +
> void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
> int oif, u32 mark)
> {
> @@ -1401,13 +1414,7 @@ void ip6_update_pmtu(struct sk_buff *skb,
> struct net *net, __be32 mtu,
> struct dst_entry *dst;
> struct flowi6 fl6;
>
> - memset(&fl6, 0, sizeof(fl6));
> - fl6.flowi6_oif = oif;
> - fl6.flowi6_mark = mark ? mark : IP6_REPLY_MARK(net, skb->mark);
> - fl6.daddr = iph->daddr;
> - fl6.saddr = iph->saddr;
> - fl6.flowlabel = ip6_flowinfo(iph);
> -
> + ip6_fill_in_flow(&fl6, net, skb, oif, mark);
> dst = ip6_route_output(net, NULL, &fl6);
> if (!dst->error)
> __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
> @@ -1417,8 +1424,22 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
>
> void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
> {
> - ip6_update_pmtu(skb, sock_net(sk), mtu,
> + struct ipv6_pinfo *np = inet6_sk(sk);
> + struct dst_entry *dst_new;
> + struct flowi6 fl6;
> + struct net *net = sock_net(sk);
> +
> + ip6_update_pmtu(skb, net, mtu,
> + sk->sk_bound_dev_if, sk->sk_mark);
> +
> + if (sk->sk_state == TCP_ESTABLISHED &&
> + !sk_dst_check(sk, np->dst_cookie)) {
sk_dst_check could hold a refcnt to the current sk->sk_dst_cache.
It has to be released.
> + ip6_fill_in_flow(&fl6, net, skb,
> sk->sk_bound_dev_if, sk->sk_mark);
> + dst_new = ip6_route_output(net, NULL, &fl6);
> + if (!IS_ERR(dst_new))
> + ip6_dst_store(sk, dst_new, NULL, NULL);
Has sk been locked before ip6_dst_store?
Some fast path may do __sk_dst_get() which does not refcnt the sk->sk_dst_cache.
However, after a quick look, it seems udpv6_sendmsg() is calling ip6_dst_store()
after release_sock also. I could be missing something.
^ permalink raw reply
* Re: [PATCH 1/1] net: Add SO_REUSEPORT_LISTEN_OFF socket option as drain mode
From: Eric Dumazet @ 2016-03-24 18:32 UTC (permalink / raw)
To: Willy Tarreau
Cc: Tolga Ceylan, Tom Herbert, cgallek, Josh Snyder, Aaron Conole,
David S. Miller, Linux Kernel Network Developers
In-Reply-To: <20160324180011.GB7585@1wt.eu>
On Thu, 2016-03-24 at 19:00 +0100, Willy Tarreau wrote:
> OK so this means we have to find a way to expand it to allow an individual
> non-privileged process to change the distribution algorithm without impacting
> other processes.
Just to clarify : Installing a BPF filter on a SO_REUSEPORT socket is
not a privileged operation.
^ permalink raw reply
* Re: [PATCH net v2 RESEND] xfrm: Fix crash observed during device unregistration and decryption
From: David Miller @ 2016-03-24 18:30 UTC (permalink / raw)
To: subashab; +Cc: netdev, steffen.klassert, eric.dumazet, jeromes
In-Reply-To: <1458794390-19788-1-git-send-email-subashab@codeaurora.org>
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Date: Wed, 23 Mar 2016 22:39:50 -0600
> A crash is observed when a decrypted packet is processed in receive
> path. get_rps_cpus() tries to dereference the skb->dev fields but it
> appears that the device is freed from the poison pattern.
...
> Following are the sequence of events observed -
>
> - Encrypted packet in receive path from netdevice is queued
> - Encrypted packet queued for decryption (asynchronous)
> - Netdevice brought down and freed
> - Packet is decrypted and returned through callback in esp_input_done
> - Packet is queued again for process in network stack using netif_rx
>
> Since the device appears to have been freed, the dereference of
> skb->dev in get_rps_cpus() leads to an unhandled page fault
> exception.
>
> Fix this by holding on to device reference when queueing packets
> asynchronously and releasing the reference on call back return.
>
> v2: Make the change generic to xfrm as mentioned by Steffen and
> update the title to xfrm
>
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH net v2 2/2] net: bcmgenet: fix skb_len in bcmgenet_xmit_single()
From: Petri Gynther @ 2016-03-24 18:27 UTC (permalink / raw)
To: netdev; +Cc: davem, f.fainelli, jaedon.shin, edumazet, Petri Gynther
In-Reply-To: <1458844041-71532-1-git-send-email-pgynther@google.com>
skb_len needs to be skb_headlen(skb) in bcmgenet_xmit_single().
Fragmented skbs can have only Ethernet + IP + TCP headers (14+20+20=54 bytes)
in the linear buffer, followed by the rest in fragments. Bumping skb_len to
ETH_ZLEN would be incorrect for this case, as it would introduce garbage
between TCP header and the fragment data.
This also works with regular/non-fragmented small packets < ETH_ZLEN bytes.
Successfully tested this on GENETv3 with 42-byte ARP frames.
For testing, I used:
ethtool -K eth0 tx-checksum-ipv4 off
ethtool -K eth0 tx-checksum-ipv6 off
echo 0 > /proc/sys/net/ipv4/tcp_timestamps
Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Petri Gynther <pgynther@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index c1c7c0e..cf6445d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1297,7 +1297,7 @@ static int bcmgenet_xmit_single(struct net_device *dev,
tx_cb_ptr->skb = skb;
- skb_len = skb_headlen(skb) < ETH_ZLEN ? ETH_ZLEN : skb_headlen(skb);
+ skb_len = skb_headlen(skb);
mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
ret = dma_mapping_error(kdev, mapping);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox