Netdev List
 help / color / mirror / Atom feed
* Re: [OOPS][3.12] BUG: unable to handle kernel NULL pointer dereference at 0000000c
From: David Miller @ 2013-11-15 22:44 UTC (permalink / raw)
  To: bhelgaas; +Cc: pomac, linux-kernel, edumazet, netdev, alex.aring
In-Reply-To: <CAErSpo4jh7pO=jhZTvHD+sT2O12Ly6V3e_oiac8gHqmdmqevkg@mail.gmail.com>

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Fri, 15 Nov 2013 15:29:53 -0700

> [+cc David, Eric, Alex, netdev]
> 
> Alex reported a similar issue at
> http://marc.info/?l=linux-netdev&m=138355719901790&w=4

Fixed by:

commit 84502b5ef9849a9694673b15c31bd3ac693010ae
Author: Steffen Klassert <steffen.klassert@secunet.com>
Date:   Wed Oct 30 11:16:28 2013 +0100

    xfrm: Fix null pointer dereference when decoding sessions
    
    On some codepaths the skb does not have a dst entry
    when xfrm_decode_session() is called. So check for
    a valid skb_dst() before dereferencing the device
    interface index. We use 0 as the device index if
    there is no valid skb_dst(), or at reverse decoding
    we use skb_iif as device interface index.
    
    Bug was introduced with git commit bafd4bd4dc
    ("xfrm: Decode sessions with output interface.").
    
    Reported-by: Meelis Roos <mroos@linux.ee>
    Tested-by: Meelis Roos <mroos@linux.ee>
    Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 4764ee4..e1a6393 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -104,10 +104,14 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
 	const struct iphdr *iph = ip_hdr(skb);
 	u8 *xprth = skb_network_header(skb) + iph->ihl * 4;
 	struct flowi4 *fl4 = &fl->u.ip4;
+	int oif = 0;
+
+	if (skb_dst(skb))
+		oif = skb_dst(skb)->dev->ifindex;
 
 	memset(fl4, 0, sizeof(struct flowi4));
 	fl4->flowi4_mark = skb->mark;
-	fl4->flowi4_oif = skb_dst(skb)->dev->ifindex;
+	fl4->flowi4_oif = reverse ? skb->skb_iif : oif;
 
 	if (!ip_is_fragment(iph)) {
 		switch (iph->protocol) {
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index dd503a3..5f8e128 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -135,10 +135,14 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
 	struct ipv6_opt_hdr *exthdr;
 	const unsigned char *nh = skb_network_header(skb);
 	u8 nexthdr = nh[IP6CB(skb)->nhoff];
+	int oif = 0;
+
+	if (skb_dst(skb))
+		oif = skb_dst(skb)->dev->ifindex;
 
 	memset(fl6, 0, sizeof(struct flowi6));
 	fl6->flowi6_mark = skb->mark;
-	fl6->flowi6_oif = skb_dst(skb)->dev->ifindex;
+	fl6->flowi6_oif = reverse ? skb->skb_iif : oif;
 
 	fl6->daddr = reverse ? hdr->saddr : hdr->daddr;
 	fl6->saddr = reverse ? hdr->daddr : hdr->saddr;

^ permalink raw reply related

* Re: [PATCH net v4] bonding: add ip checks when store ip target
From: David Miller @ 2013-11-15 22:44 UTC (permalink / raw)
  To: dingtianhong; +Cc: fubar, andy, nikolay, vfalico, netdev
In-Reply-To: <52857FE2.5060400@huawei.com>

From: Ding Tianhong <dingtianhong@huawei.com>
Date: Fri, 15 Nov 2013 09:58:58 +0800

> From 513509eecdee98d72668219aead9784d56dbad0f Mon Sep 17 00:00:00 2001
> From: Wang Weidong <wangweidong1@huawei.com>
> Date: Fri, 15 Nov 2013 10:34:30 -0500
> Subject: [PATCH] bonding: add ip checks when store ip target
> 
> I met a Bug when I add ip target with the wrong ip address:
> 
> echo +500.500.500.500 > /sys/class/net/bond0/bonding/arp_ip_target
> 
> the wrong ip address will transfor to 245.245.245.244 and add
> to the ip target success, it is uncorrect, so I add checks to avoid
> adding wrong address.
> 
> The in4_pton() will set wrong ip address to 0.0.0.0, it will return by
> the next check and will not add to ip target.
> 
> v2
> According Veaceslav's opinion, simplify the code.
> 
> v3
> According Veaceslav's opinion, add broadcast check and make a micro
> definition to package it.
> 
> v4
> Solve the problem of the format which David point out.
> 
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Suggested-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH] net: sctp: recover a tranport when an ack comes
From: Vlad Yasevich @ 2013-11-15 22:48 UTC (permalink / raw)
  To: Chang, nhorman; +Cc: davem, linux-sctp, netdev, linux-kernel, dreibh, ernstgr
In-Reply-To: <52869A6C.8060900@gmail.com>

On 11/15/2013 05:04 PM, Chang wrote:
>
> On 11/15/2013 03:34 AM, Vlad Yasevich wrote:
>> I don't think this is right.  The spec states:
>>  8.  ACKs for retransmissions do not transition a PF destination back
>>        to Active state, since a sender cannot disambiguate whether the
>>        ack was for the original transmission or the retransmission(s).
>>
> Could you please reconsider my proposal?
>
> In the rule 8, it clearly specifies ACKs for *retransmission* do not
> transition ... But those chunks were not retransmitted!
>
> Every transport maintains its own [transport->transmitted] queue, when
> retransmit happens (no matter time_out or fast_retransmit). The chunk
> would be removed from the queue and moved to the sctp_outq->retransmit.
>

Yes,  this is their temporary holding area.  Once the chunks are
transmitted again, they are placed on the transports transmitted list.
See sctp_outq_flush_rtx().

> static void sctp_check_transmitted(...) {
> ...
>     if (transport) {  /*<=======This proves that its not the
> outq->retransmit (the retransmitted queue)*/

Retransmit queue may hold more data then can be drained in a single
push.  Thus is usually holds data that is pending retransmission, but
is not retransmitted yet.  If a late SACK arrives acknowledging this
data, we need to properly mark it.  That is what this code tries
to do.

-vlad
>         if (bytes_acked) {
>         ...
>         if((transport->state in [INACTIVE, UNCONFIRMED, PF]...)
>             sctp_assoc_control_transport(..., SCTP_TRANSPORT_UP).
>
>
> In addition, if its not appropriate to transition PF->ACTIVE, why is it
> appropriate to transition INACTIVE->ACTIVE (the original implementation).
>>
>> Now, the proper way to this would would be modify
>> sctp_assoc_control_transport() to transition the transport state to
>> ACTIVE if it was PF transport that was chosen to send data.
>>
>> -vlad
>>
>>> ---
>>>   net/sctp/outqueue.c |    1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>> index 94df758..2557fa5 100644
>>> --- a/net/sctp/outqueue.c
>>> +++ b/net/sctp/outqueue.c
>>> @@ -1517,6 +1517,7 @@ static void sctp_check_transmitted(struct
>>> sctp_outq *q,
>>>                * active if it is not so marked.
>>>                */
>>>               if ((transport->state == SCTP_INACTIVE ||
>>> +                 transport->state == SCTP_PF ||
>>>                    transport->state == SCTP_UNCONFIRMED) &&
>>>                   sctp_cmp_addr_exact(&transport->ipaddr, saddr)) {
>>>                   sctp_assoc_control_transport(
>>>
>>
>

^ permalink raw reply

* Re: [PATCH net v3 0/2] macvlan: disable LRO on lowerdev instead of a macvlan
From: David Miller @ 2013-11-15 22:56 UTC (permalink / raw)
  To: mkubecek; +Cc: netdev, kaber, john.r.fastabend, vyasevich
In-Reply-To: <cover.1384492269.git.mkubecek@suse.cz>

From: Michal Kubecek <mkubecek@suse.cz>
Date: Fri, 15 Nov 2013 06:18:30 +0100 (CET)

> A customer of ours encountered a problem with LRO on an ixgbe network
> card. Analysis showed that it was a known conflict of forwarding and LRO
> but the forwarding was enabled in an LXC container where only a macvlan
> was, not the ethernet device itself.
> 
> I believe the solution is exactly the same as what we do for "normal"
> (802.1q) VLAN devices: if dev_disable_lro() is called for such device,
> LRO is disabled on the underlying "real" device instead.
> 
> v2: adapt to changes merged from net-next
> 
> v3: use BUG() in macvlan_dev_real_dev() if compiled without macvlan

Series applied, thanks Michal.

^ permalink raw reply

* [PATCH 1/4] net/phy: Add VSC8234 support
From: Shruti Kanetkar @ 2013-11-15 22:58 UTC (permalink / raw)
  To: davem, netdev; +Cc: Andy Fleming, Kumar Gala, Shruti Kanetkar

From: Andy Fleming <afleming@gmail.com>

Vitesse VSC8234 is quad port 10/100/1000BASE-T PHY
with SGMII and SERDES MAC interfaces.

Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
 drivers/net/phy/vitesse.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 69b482b..59e3b58 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -54,6 +54,7 @@
 #define MII_VSC8221_AUXCONSTAT_INIT	0x0004 /* need to set this bit? */
 #define MII_VSC8221_AUXCONSTAT_RESERVED	0x0004
 
+#define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
 #define PHY_ID_VSC8221			0x000fc550
 #define PHY_ID_VSC8211			0x000fc4b0
@@ -118,7 +119,8 @@ static int vsc82xx_config_intr(struct phy_device *phydev)
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		err = phy_write(phydev, MII_VSC8244_IMASK,
-			phydev->drv->phy_id == PHY_ID_VSC8244 ?
+			(phydev->drv->phy_id == PHY_ID_VSC8234 ||
+			 phydev->drv->phy_id == PHY_ID_VSC8244) ?
 				MII_VSC8244_IMASK_MASK :
 				MII_VSC8221_IMASK_MASK);
 	else {
@@ -149,9 +151,21 @@ static int vsc8221_config_init(struct phy_device *phydev)
 	 */
 }
 
-/* Vitesse 824x */
+/* Vitesse 82xx */
 static struct phy_driver vsc82xx_driver[] = {
 {
+	.phy_id         = PHY_ID_VSC8234,
+	.name           = "Vitesse VSC8234",
+	.phy_id_mask    = 0x000ffff0,
+	.features       = PHY_GBIT_FEATURES,
+	.flags          = PHY_HAS_INTERRUPT,
+	.config_init    = &vsc824x_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc824x_ack_interrupt,
+	.config_intr    = &vsc82xx_config_intr,
+	.driver         = { .owner = THIS_MODULE,},
+}, {
 	.phy_id		= PHY_ID_VSC8244,
 	.name		= "Vitesse VSC8244",
 	.phy_id_mask	= 0x000fffc0,
@@ -207,6 +221,7 @@ module_init(vsc82xx_init);
 module_exit(vsc82xx_exit);
 
 static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
+	{ PHY_ID_VSC8234, 0x000ffff0 },
 	{ PHY_ID_VSC8244, 0x000fffc0 },
 	{ PHY_ID_VSC8221, 0x000ffff0 },
 	{ PHY_ID_VSC8211, 0x000ffff0 },
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH 2/4] net/phy: Add VSC8574 support
From: Shruti Kanetkar @ 2013-11-15 22:58 UTC (permalink / raw)
  To: davem, netdev; +Cc: shaohui xie, Andy Fleming, Shruti Kanetkar
In-Reply-To: <1384556305-17133-1-git-send-email-Shruti@Freescale.com>

From: shaohui xie <Shaohui.Xie@freescale.com>

The VSC8574 is a quad-port Gigabit Ethernet transceiver with four SerDes
interfaces for quad-port dual media capability.

Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
 drivers/net/phy/vitesse.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 59e3b58..31b3148 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -56,6 +56,7 @@
 
 #define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
+#define PHY_ID_VSC8574			0x000704a0
 #define PHY_ID_VSC8221			0x000fc550
 #define PHY_ID_VSC8211			0x000fc4b0
 
@@ -120,7 +121,8 @@ static int vsc82xx_config_intr(struct phy_device *phydev)
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		err = phy_write(phydev, MII_VSC8244_IMASK,
 			(phydev->drv->phy_id == PHY_ID_VSC8234 ||
-			 phydev->drv->phy_id == PHY_ID_VSC8244) ?
+			 phydev->drv->phy_id == PHY_ID_VSC8244 ||
+			 phydev->drv->phy_id == PHY_ID_VSC8574) ?
 				MII_VSC8244_IMASK_MASK :
 				MII_VSC8221_IMASK_MASK);
 	else {
@@ -178,6 +180,18 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_intr	= &vsc82xx_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
 }, {
+	.phy_id         = PHY_ID_VSC8574,
+	.name           = "Vitesse VSC8574",
+	.phy_id_mask    = 0x000ffff0,
+	.features       = PHY_GBIT_FEATURES,
+	.flags          = PHY_HAS_INTERRUPT,
+	.config_init    = &vsc824x_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc824x_ack_interrupt,
+	.config_intr    = &vsc82xx_config_intr,
+	.driver         = { .owner = THIS_MODULE,},
+}, {
 	/* Vitesse 8221 */
 	.phy_id		= PHY_ID_VSC8221,
 	.phy_id_mask	= 0x000ffff0,
@@ -223,6 +237,7 @@ module_exit(vsc82xx_exit);
 static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
 	{ PHY_ID_VSC8234, 0x000ffff0 },
 	{ PHY_ID_VSC8244, 0x000fffc0 },
+	{ PHY_ID_VSC8574, 0x000ffff0 },
 	{ PHY_ID_VSC8221, 0x000ffff0 },
 	{ PHY_ID_VSC8211, 0x000ffff0 },
 	{ }
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH 3/4] net/phy: Add VSC8662 support
From: Shruti Kanetkar @ 2013-11-15 22:58 UTC (permalink / raw)
  To: davem, netdev; +Cc: Sandeep Singh, Sandeep Singh, Andy Fleming, Shruti Kanetkar
In-Reply-To: <1384556305-17133-1-git-send-email-Shruti@Freescale.com>

From: Sandeep Singh <sandeep@freescale.com>

Vitesse VSC8662 is Dual Port 10/100/1000Base-T Phy
Its register set and features are similar to other Vitesse Phys.

Signed-off-by: Sandeep Singh <Sandeep@freescale.com>
Signed-off-by: Andy Fleming <afleming@gmail.com>
Signed-off-by: Shruti Kanetkar <Shruti@Freescale.com>
---
 drivers/net/phy/vitesse.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 31b3148..40406bc 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -57,6 +57,7 @@
 #define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
 #define PHY_ID_VSC8574			0x000704a0
+#define PHY_ID_VSC8662			0x00070660
 #define PHY_ID_VSC8221			0x000fc550
 #define PHY_ID_VSC8211			0x000fc4b0
 
@@ -192,6 +193,18 @@ static struct phy_driver vsc82xx_driver[] = {
 	.config_intr    = &vsc82xx_config_intr,
 	.driver         = { .owner = THIS_MODULE,},
 }, {
+	.phy_id         = PHY_ID_VSC8662,
+	.name           = "Vitesse VSC8662",
+	.phy_id_mask    = 0x000ffff0,
+	.features       = PHY_GBIT_FEATURES,
+	.flags          = PHY_HAS_INTERRUPT,
+	.config_init    = &vsc824x_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc824x_ack_interrupt,
+	.config_intr    = &vsc82xx_config_intr,
+	.driver         = { .owner = THIS_MODULE,},
+}, {
 	/* Vitesse 8221 */
 	.phy_id		= PHY_ID_VSC8221,
 	.phy_id_mask	= 0x000ffff0,
@@ -238,6 +251,7 @@ static struct mdio_device_id __maybe_unused vitesse_tbl[] = {
 	{ PHY_ID_VSC8234, 0x000ffff0 },
 	{ PHY_ID_VSC8244, 0x000fffc0 },
 	{ PHY_ID_VSC8574, 0x000ffff0 },
+	{ PHY_ID_VSC8662, 0x000ffff0 },
 	{ PHY_ID_VSC8221, 0x000ffff0 },
 	{ PHY_ID_VSC8211, 0x000ffff0 },
 	{ }
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH 4/4] net/phy: Add the autocross feature for forced links on VSC82x4
From: Shruti Kanetkar @ 2013-11-15 22:58 UTC (permalink / raw)
  To: davem, netdev; +Cc: Madalin Bucur, Shruti Kanetkar
In-Reply-To: <1384556305-17133-1-git-send-email-Shruti@Freescale.com>

From: Madalin Bucur <madalin.bucur@freescale.com>

Add auto-MDI/MDI-X capability for forced (autonegotiation disabled)
10/100 Mbps speeds on Vitesse VSC82x4 PHYs. Exported previously static
function genphy_setup_forced() required by the new config_aneg handler
in the Vitesse PHY module.

Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
Signed-off-by: Shruti Kanetkar <Shruti@freescale.com>
---
 drivers/net/phy/phy_device.c |  4 +--
 drivers/net/phy/vitesse.c    | 75 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/phy.h          |  1 +
 3 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74630e9..d6447b3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -697,7 +697,7 @@ static int genphy_config_advert(struct phy_device *phydev)
  *   to the values in phydev. Assumes that the values are valid.
  *   Please see phy_sanitize_settings().
  */
-static int genphy_setup_forced(struct phy_device *phydev)
+int genphy_setup_forced(struct phy_device *phydev)
 {
 	int err;
 	int ctl = 0;
@@ -716,7 +716,7 @@ static int genphy_setup_forced(struct phy_device *phydev)
 
 	return err;
 }
-
+EXPORT_SYMBOL(genphy_setup_forced);
 
 /**
  * genphy_restart_aneg - Enable and Restart Autonegotiation
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 40406bc..508e435 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -3,7 +3,7 @@
  *
  * Author: Kriston Carson
  *
- * Copyright (c) 2005, 2009 Freescale Semiconductor, Inc.
+ * Copyright (c) 2005, 2009, 2011 Freescale Semiconductor, Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -18,6 +18,11 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 
+/* Vitesse Extended Page Magic Register(s) */
+#define MII_VSC82X4_EXT_PAGE_16E	0x10
+#define MII_VSC82X4_EXT_PAGE_17E	0x11
+#define MII_VSC82X4_EXT_PAGE_18E	0x12
+
 /* Vitesse Extended Control Register 1 */
 #define MII_VSC8244_EXT_CON1           0x17
 #define MII_VSC8244_EXTCON1_INIT       0x0000
@@ -54,6 +59,9 @@
 #define MII_VSC8221_AUXCONSTAT_INIT	0x0004 /* need to set this bit? */
 #define MII_VSC8221_AUXCONSTAT_RESERVED	0x0004
 
+/* Vitesse Extended Page Access Register */
+#define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
+
 #define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
 #define PHY_ID_VSC8574			0x000704a0
@@ -154,6 +162,63 @@ static int vsc8221_config_init(struct phy_device *phydev)
 	 */
 }
 
+/* vsc82x4_config_autocross_enable - Enable auto MDI/MDI-X for forced links
+ * @phydev: target phy_device struct
+ *
+ * Enable auto MDI/MDI-X when in 10/100 forced link speeds by writing
+ * special values in the VSC8234/VSC8244 extended reserved registers
+ */
+static int vsc82x4_config_autocross_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed > SPEED_100)
+		return 0;
+
+	/* map extended registers set 0x10 - 0x1e */
+	ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x52b5);
+	if (ret >= 0)
+		ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_18E, 0x0012);
+	if (ret >= 0)
+		ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_17E, 0x2803);
+	if (ret >= 0)
+		ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_16E, 0x87fa);
+	/* map standard registers set 0x10 - 0x1e */
+	if (ret >= 0)
+		ret = phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x0000);
+	else
+		phy_write(phydev, MII_VSC82X4_EXT_PAGE_ACCESS, 0x0000);
+
+	return ret;
+}
+
+/* vsc82x4_config_aneg - restart auto-negotiation or write BMCR
+ * @phydev: target phy_device struct
+ *
+ * Description: If auto-negotiation is enabled, we configure the
+ *   advertising, and then restart auto-negotiation.  If it is not
+ *   enabled, then we write the BMCR and also start the auto
+ *   MDI/MDI-X feature
+ */
+static int vsc82x4_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Enable auto MDI/MDI-X when in 10/100 forced link speeds by
+	 * writing special values in the VSC8234 extended reserved registers
+	 */
+	if (phydev->autoneg != AUTONEG_ENABLE && phydev->speed <= SPEED_100) {
+		ret = genphy_setup_forced(phydev);
+
+		if (ret < 0) /* error */
+			return ret;
+
+		return vsc82x4_config_autocross_enable(phydev);
+	}
+
+	return genphy_config_aneg(phydev);
+}
+
 /* Vitesse 82xx */
 static struct phy_driver vsc82xx_driver[] = {
 {
@@ -163,7 +228,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.features       = PHY_GBIT_FEATURES,
 	.flags          = PHY_HAS_INTERRUPT,
 	.config_init    = &vsc824x_config_init,
-	.config_aneg    = &genphy_config_aneg,
+	.config_aneg    = &vsc82x4_config_aneg,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc824x_ack_interrupt,
 	.config_intr    = &vsc82xx_config_intr,
@@ -175,7 +240,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= &vsc824x_config_init,
-	.config_aneg	= &genphy_config_aneg,
+	.config_aneg	= &vsc82x4_config_aneg,
 	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &vsc824x_ack_interrupt,
 	.config_intr	= &vsc82xx_config_intr,
@@ -187,7 +252,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.features       = PHY_GBIT_FEATURES,
 	.flags          = PHY_HAS_INTERRUPT,
 	.config_init    = &vsc824x_config_init,
-	.config_aneg    = &genphy_config_aneg,
+	.config_aneg    = &vsc82x4_config_aneg,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc824x_ack_interrupt,
 	.config_intr    = &vsc82xx_config_intr,
@@ -199,7 +264,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.features       = PHY_GBIT_FEATURES,
 	.flags          = PHY_HAS_INTERRUPT,
 	.config_init    = &vsc824x_config_init,
-	.config_aneg    = &genphy_config_aneg,
+	.config_aneg    = &vsc82x4_config_aneg,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc824x_ack_interrupt,
 	.config_intr    = &vsc82xx_config_intr,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 64ab823..48a4dc3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -559,6 +559,7 @@ static inline int phy_read_status(struct phy_device *phydev) {
 	return phydev->drv->read_status(phydev);
 }
 
+int genphy_setup_forced(struct phy_device *phydev);
 int genphy_restart_aneg(struct phy_device *phydev);
 int genphy_config_aneg(struct phy_device *phydev);
 int genphy_update_link(struct phy_device *phydev);
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH v3] net: Do not include padding in TCP GRO checksum
From: Alexander Duyck @ 2013-11-15 23:00 UTC (permalink / raw)
  To: davem; +Cc: netdev, edumazet, herbert

In some recent tests I found the TCP checksum was being treated as valid
for certain frames with padding on them.  On closer inspection I found the
issue was that GRO was using the skb->len instead of the length recorded in
the IP/IPv6 header to determine the number of bytes to checksum.  As such
padded frames that actually had invalid checksums generated by adding the
padding to the checksum were being incorrectly tagged as valid.

This change corrects that by using the tot_len from IPv4 headers and the
payload_len from IPv6 headers to compute the total number of bytes to be
included in the checksum.

To address the fact that skb->csum is invalid when a padded frame is
received I have updated the code to fall though to the CHECKSUM_NONE path
for CHECKSUM_COMPLETE frames that contain padding.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v2: Update byte ordering of tot_len and payload_len so it is in host order.
    Updated CHECKSUM_COMPLETE path so it falls back through CHECKSUM_NONE for
    padded frames since this is how it is handled in ip_rcv.

    I have tested and verified the CHECKSUM_NONE path works, but I don't have
    any adapters that generate CHECKSUM_COMPLETE to test with.
v3: Added check to handle case where length is greater than skb_gro_len.

 net/ipv4/tcp_offload.c   |   30 +++++++++++++++++++++---------
 net/ipv6/tcpv6_offload.c |   31 +++++++++++++++++++++----------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..b32f6c3 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -273,26 +273,38 @@ static int tcp_v4_gso_send_check(struct sk_buff *skb)
 static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *skb)
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->tot_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offsets */
+	length += skb_network_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v4_check(length, iph->saddr, iph->daddr,
+					   skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
+					  length, IPPROTO_TCP, 0);
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index c1097c7..f6047cc 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -36,27 +36,38 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->payload_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offset due to extension headers */
+	length += skb_transport_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v6_check(length, &iph->saddr, &iph->daddr,
+					  skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr,
-						    skb_gro_len(skb),
-						    IPPROTO_TCP, 0));
+						    length, IPPROTO_TCP, 0));
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;

^ permalink raw reply related

* Re: [OOPS][3.12] BUG: unable to handle kernel NULL pointer dereference at 0000000c
From: Ian Kumlien @ 2013-11-15 23:39 UTC (permalink / raw)
  To: David Miller; +Cc: bhelgaas, pomac, linux-kernel, edumazet, netdev, alex.aring
In-Reply-To: <20131115.174426.1448825246187940374.davem@davemloft.net>

On Fri, Nov 15, 2013 at 05:44:26PM -0500, David Miller wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Fri, 15 Nov 2013 15:29:53 -0700
> 
> > [+cc David, Eric, Alex, netdev]
> > 
> > Alex reported a similar issue at
> > http://marc.info/?l=linux-netdev&m=138355719901790&w=4
> 
> Fixed by:
> 
> commit 84502b5ef9849a9694673b15c31bd3ac693010ae
> Author: Steffen Klassert <steffen.klassert@secunet.com>
> Date:   Wed Oct 30 11:16:28 2013 +0100

Cherry-picked, compiled and preparing for reboot - thanks!

Shouldn't this be queued up in stable sometime soonish?

(Sorry for the change of email address, i had forgot to switch the
configurations in mutt)

^ permalink raw reply

* Re: [OOPS][3.12] BUG: unable to handle kernel NULL pointer dereference at 0000000c
From: David Miller @ 2013-11-15 23:57 UTC (permalink / raw)
  To: pomac
  Cc: bhelgaas, pomac, linux-kernel, edumazet, netdev, alex.aring,
	steffen.klassert
In-Reply-To: <20131115233919.GB11450@pomac.netswarm.net>

From: Ian Kumlien <pomac@demius.net>
Date: Sat, 16 Nov 2013 00:39:19 +0100

> On Fri, Nov 15, 2013 at 05:44:26PM -0500, David Miller wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> Date: Fri, 15 Nov 2013 15:29:53 -0700
>> 
>> > [+cc David, Eric, Alex, netdev]
>> > 
>> > Alex reported a similar issue at
>> > http://marc.info/?l=linux-netdev&m=138355719901790&w=4
>> 
>> Fixed by:
>> 
>> commit 84502b5ef9849a9694673b15c31bd3ac693010ae
>> Author: Steffen Klassert <steffen.klassert@secunet.com>
>> Date:   Wed Oct 30 11:16:28 2013 +0100
> 
> Cherry-picked, compiled and preparing for reboot - thanks!
> 
> Shouldn't this be queued up in stable sometime soonish?
> 
> (Sorry for the change of email address, i had forgot to switch the
> configurations in mutt)

Steffen is in charge of IPSEC -stable submissions, please ask him
:-)

^ permalink raw reply

* Re: [PATCH] sh_eth: call phy_scan_fixups() after PHY reset
From: Florian Fainelli @ 2013-11-16  0:04 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: David Miller, nobuhiro.iwamatsu.yj, netdev, linux-sh,
	laurent.pinchart+renesas
In-Reply-To: <5286C054.9090602@cogentembedded.com>

2013/11/15 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 11/05/2013 11:01 PM, David Miller wrote:
>
>>>     David, will you ever reply to this mail?
>
>
>> I really haven't had the time with my backlog, and it's been so long ago
>> that I've forgotten all of the context.
>
>
>    Hm, I thought I provided enough context and also that's what mail
> archives are for... :-)
>
>
>> Sorry, someone will have to start the conversation up anew.

[snip]

>
>    Now, about the ways to deal with this issue that I see.
>    First I thought about using PHY platform fixup mechanism which is already
> hooked up to a PHY reset in phy_mii_ioctl() (the code is somewhat naive
> though as it doesn't wait for the reset completion before calling fixups and
> the driver's config_init() method).

That's something that needs fixing and also a dedicated helper or such
should be used because we are potentially messing up with the
configuration the PHY library thinks it has applied to the PHY (e.g:
disabling auto-negotiation etc...).

> All I needed is calling fixups after
> direct PHY reset done via phy_write() but I didn't want to add a hook there
> due to that reset completion wait loop that was obviously necessary (this
> function is simple *inline*), so went for a simplistic local
> phy_scan_fixups() call (that function was helpfully already exported
> although not called by anyone except phylib itself) after PHY reset in the
> 'sh_eth' driver which you saw in my patch and which you didn't like; I've
> also coded the PHY platform fixup which got successfully merged via
> arch/arm/mach-shmobile/ subtree at that time. Your argument was that the
> driver is going against the control flow provided by phylib (it's not alone
> doing PHY reset, I should note) and so you wanted me to embed everything
> into phylib control flow. I replied that I fear the unexpected consequencies
> of doing compulsory PHY reset for every driver using phylib (that's how I
> understood your idea at least)... then, after a long pause, I suggested to
> code phy_reset() function within phylib which the drivers that currently do
> reset their PHYs could use instead of open coding the reset bit polling
> loop, etc. and which would include a hook for the platform fixups and PHY
> driver config_init() call. I'd still like to hear your opinion on this
> approach -- which is less invasive.
>    What I can also do in this case is again ignore ETH_LINK signal in the
> 'sh_eth' driver and stop caring about the LED functions matching to what's
> designed for the boards. This doesn't need PHY platform fixup or any change
> in phylib and Ethernet driver.

This signal is probably present for a good reason: avoiding expensive
MDIO register access, and if that works for most platforms why not
keeping it?

>    What I also can do is stop resetting PHY in the 'sh_eth' driver (only
> getting it out of sleep for which the reset doesn't seem necessary),
> although we'd still need the PHY platform fixup for the PHY resets done via
> phy_mii_ioctl()... I'm not so much familiar with the driver to be sure it
> won't hurt (the driver is used on e.g. some SH platforms I don't get any
> access to) and I couldn't get any useful input from the driver's primary
> author -- but perhaps it's really not necessary. What do you think?

Quite a lot of PHYs actually require a reset through BMCR_RESET when
resuming from S2/S3 sleep states and even if they do not, it does not
hurt doing so, I would welcome a generic solution to do that which
would not circumvent the libphy state machine and APIs. So the general
idea would be to:

- provide a helper, e.g: phy_reset(phydev) which:
- toggles BMCR_RESET, waits for it to be clear
- call fixups on the phy_device
- call config_init on the phy_device
- restores any kind of duplex/autoneg settings we enabled/disabled
- resume the PHY state machine at the appropriate state
--
Florian

^ permalink raw reply

* [PATCH tip/core/rcu 05/14] ipv4/ip_socketglue: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
ip_ra_control() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv4/ip_sockglue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index d9c4f113d709..a0e7f176e9c8 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -269,7 +269,8 @@ int ip_ra_control(struct sock *sk, unsigned char on,
 			}
 			/* dont let ip_call_ra_chain() use sk again */
 			ra->sk = NULL;
-			rcu_assign_pointer(*rap, ra->next);
+			/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+			ACCESS_ONCE(*rap) = ra->next;
 			spin_unlock_bh(&ip_ra_lock);
 
 			if (ra->destructor)
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 07/14] ipv6/ip6_gre: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
ip6gre_tunnel_unlink() is legitimate: It is assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/ip6_gre.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 6b26e9feafb9..7bc9e1b3283e 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -276,7 +276,8 @@ static void ip6gre_tunnel_unlink(struct ip6gre_net *ign, struct ip6_tnl *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+			ACCESS_ONCE(*tp) = t->next;
 			break;
 		}
 	}
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 09/14] mac80211: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, John W. Linville, Johannes Berg,
	David S. Miller, linux-wireless, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the uses in
sta_info_hash_del() are legitimate: They are assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 net/mac80211/sta_info.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aeb967a0aeed..494f03c0831f 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -74,8 +74,8 @@ static int sta_info_hash_del(struct ieee80211_local *local,
 	if (!s)
 		return -ENOENT;
 	if (s == sta) {
-		rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)],
-				   s->hnext);
+		/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+		ACCESS_ONCE(local->sta_hash[STA_HASH(sta->sta.addr)]) = s->hnext;
 		return 0;
 	}
 
@@ -84,7 +84,8 @@ static int sta_info_hash_del(struct ieee80211_local *local,
 		s = rcu_dereference_protected(s->hnext,
 					lockdep_is_held(&local->sta_mtx));
 	if (rcu_access_pointer(s->hnext)) {
-		rcu_assign_pointer(s->hnext, sta->hnext);
+		/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+		ACCESS_ONCE(s->hnext) = sta->hnext;
 		return 0;
 	}
 
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 10/14] bridge/br_mdb: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: Stephen Hemminger, tglx, laijs, edumazet, peterz, fweisbec,
	bridge, josh, rostedt, David S. Miller, dhowells, sbw, niv,
	netdev, mathieu.desnoyers, dipankar, darren, akpm,
	Paul E. McKenney, mingo
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
__br_mdb_del() is legitimate: They are assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences these false positives by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
---
 net/bridge/br_mdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 85a09bb5ca51..de7197ba8695 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -447,7 +447,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 		if (p->port->state == BR_STATE_DISABLED)
 			goto unlock;
 
-		rcu_assign_pointer(*pp, p->next);
+		ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */
 		hlist_del_init(&p->mglist);
 		del_timer(&p->timer);
 		call_rcu_bh(&p->rcu, br_multicast_free_pg);
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 11/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: Stephen Hemminger, tglx, laijs, edumazet, peterz, fweisbec,
	bridge, josh, rostedt, David S. Miller, dhowells, sbw, niv,
	netdev, mathieu.desnoyers, dipankar, darren, akpm,
	Paul E. McKenney, mingo
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the uses in
bond_change_active_slave() and __bond_release_one() are legitimate:
They are assigning a pointer to an element from an RCU-protected list
(or a NULL pointer), and all elements of this list are already visible
to caller.

This commit therefore silences these false positives either by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett, or by using RCU_INIT_POINTER() for NULL pointer assignments.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
---
 drivers/net/bonding/bond_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 72df399c4ab3..bbd7fd3e46fe 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -890,7 +890,8 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 		if (new_active)
 			bond_set_slave_active_flags(new_active);
 	} else {
-		rcu_assign_pointer(bond->curr_active_slave, new_active);
+		/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+		ACCESS_ONCE(bond->curr_active_slave) = new_active;
 	}
 
 	if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
@@ -1801,7 +1802,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 	}
 
 	if (all) {
-		rcu_assign_pointer(bond->curr_active_slave, NULL);
+		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
 	} else if (oldcurrent == slave) {
 		/*
 		 * Note that we hold RTNL over this sequence, so there
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 03/14] bridge: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, Stephen Hemminger, David S. Miller, bridge,
	netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the uses in
br_multicast_del_pg() and br_multicast_new_port_group() are legitimate:
They are assigning a pointer to an element from an RCU-protected list,
and all elements of this list are already visible to caller.

This commit therefore silences these false positives by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
---
 net/bridge/br_multicast.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index d1c578630678..bcc4bbc16498 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -267,7 +267,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
 		if (p != pg)
 			continue;
 
-		rcu_assign_pointer(*pp, p->next);
+		ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */
 		hlist_del_init(&p->mglist);
 		del_timer(&p->timer);
 		call_rcu_bh(&p->rcu, br_multicast_free_pg);
@@ -646,7 +646,7 @@ struct net_bridge_port_group *br_multicast_new_port_group(
 	p->addr = *group;
 	p->port = port;
 	p->state = state;
-	rcu_assign_pointer(p->next, next);
+	ACCESS_ONCE(p->next) = next; /* OK: Both --rcu and visible. */
 	hlist_add_head(&p->mglist, &port->mglist);
 	setup_timer(&p->timer, br_multicast_port_group_expired,
 		    (unsigned long)p);
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 06/14] ipv6/ip6_tunnel: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
ip6_tnl_unlink() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/ip6_tunnel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 61355f7f4da5..2bea7a4e49ed 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -245,7 +245,8 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+			ACCESS_ONCE(*tp) = t->next;
 			break;
 		}
 	}
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 12/14] bonding/bond_alb.c: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
bond_alb_handle_active_change() is legitimate: It is assigning a pointer
to an element from an RCU-protected list, and all elements of this list
are already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 drivers/net/bonding/bond_alb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 91f179d5135c..67d3b2893aa3 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1667,7 +1667,8 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
 	}
 
 	swap_slave = bond->curr_active_slave;
-	rcu_assign_pointer(bond->curr_active_slave, new_slave);
+	/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+	ACCESS_ONCE(bond->curr_active_slave) = new_slave;
 
 	if (!new_slave || list_empty(&bond->slave_list))
 		return;
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 08/14] ipv6/sit: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
ipip6_tunnel_unlink() is legitimate: It is assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: netdev@vger.kernel.org
---
 net/ipv6/sit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 7ee5cb96db34..9b976a4b463d 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -157,7 +157,8 @@ static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
 	     (iter = rtnl_dereference(*tp)) != NULL;
 	     tp = &iter->next) {
 		if (t == iter) {
-			rcu_assign_pointer(*tp, t->next);
+			/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+			ACCESS_ONCE(*tp) = t->next;
 			break;
 		}
 	}
-- 
1.8.1.5

^ permalink raw reply related

* [PATCH tip/core/rcu 04/14] decnet: Apply ACCESS_ONCE() to avoid sparse false positive
From: Paul E. McKenney @ 2013-11-16  0:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
	Paul E. McKenney, David S. Miller, Thomas Graf, Gao feng,
	Stephen Hemminger, linux-decnet-user, netdev
In-Reply-To: <1384562417-817-1-git-send-email-paulmck@linux.vnet.ibm.com>

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces.  This also rejects __rcu,
which is almost always the right thing to do.  However, the use in
dn_insert_route() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Stephen Hemminger <shemminger@vyatta.com>
Cc: linux-decnet-user@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
 net/decnet/dn_route.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index fe32388ea24f..a6ef8b025035 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -344,8 +344,9 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
 		if (compare_keys(&rth->fld, &rt->fld)) {
 			/* Put it first */
 			*rthp = rth->dst.dn_next;
-			rcu_assign_pointer(rth->dst.dn_next,
-					   dn_rt_hash_table[hash].chain);
+			/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+			ACCESS_ONCE(rth->dst.dn_next) =
+					   dn_rt_hash_table[hash].chain;
 			rcu_assign_pointer(dn_rt_hash_table[hash].chain, rth);
 
 			dst_use(&rth->dst, now);
@@ -358,7 +359,8 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
 		rthp = &rth->dst.dn_next;
 	}
 
-	rcu_assign_pointer(rt->dst.dn_next, dn_rt_hash_table[hash].chain);
+	/* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+	ACCESS_ONCE(rt->dst.dn_next) = dn_rt_hash_table[hash].chain;
 	rcu_assign_pointer(dn_rt_hash_table[hash].chain, rt);
 
 	dst_use(&rt->dst, now);
-- 
1.8.1.5

^ permalink raw reply related

* Re: [PATCH] sh_eth: call phy_scan_fixups() after PHY reset
From: Sergei Shtylyov @ 2013-11-16  0:46 UTC (permalink / raw)
  To: David Miller
  Cc: nobuhiro.iwamatsu.yj, netdev, linux-sh, laurent.pinchart+renesas
In-Reply-To: <20131105.150153.251029756944194421.davem@davemloft.net>

Hello.

On 11/05/2013 11:01 PM, David Miller wrote:

>>     David, will you ever reply to this mail?

> I really haven't had the time with my backlog, and it's been so long ago
> that I've forgotten all of the context.

    Hm, I thought I provided enough context and also that's what mail archives 
are for... :-)

> Sorry, someone will have to start the conversation up anew.

    Anyway, here's the story told anew, with all the gory details. :-)
    Many Renesas R-Car development boards use Micrel KSZ8041 as a PHY for the 
SoC Ethernet device. This PHY has bits 14-15 of the PHY control register 1 (at 
address 0x1E) selecting LED mode for 2 LED outputs: value 00 means LED0 acting 
as LINK/ACTIVITY and LED1 as SPEED, value 01 means LED0 acting as LINK and 
LED1 as ACTIVITY; these bits are subject to the PHY reset via BMCR and get 
reset to 00 -- this is IMO bad design since it makes more sense for the LED 
mode to be pin-strapped from the external switches and so not changed on every 
PHY reset.
    Now LED0 is also connected to the sideband ETH_LINK signal of the SoC 
Ethernet device which indicates the link state to the Ethernet core (so called 
feLic); strictly speaking, it's not needed by the driver as it can receive the 
link state from the PHY via phylib. On the BOCK-W board we had the Ethenet 
LEDs labelled matching the 00 value of the bits 14-15, we had no issues with 
the LED mode changing after reset but we had an issue with ETH_LINK bouncing 
off/on after each packet with which we dealt by completely ignoring this 
signal in the 'sh_eth' driver (this is possible using 'no_ether_link' driver's 
platform data field). Now, on the newer Lager/ Koelsch boards we have the 
Ethenet LEDs labelled matching the 01 value of the bits 14-15, so we should 
get the valid ETH_LINK signal (except it's inverted but we deal with this 
using 'ether_link_active_low' platform data field)... if the 'sh_eth' driver 
didn't reset the PHY each time the device is opened, which gets us back to the 
value of 00 of the PHY LED control bits and so to the issue of bouncing 
ETH_LINK signal (that unfortunately leads to an NFS timeout with NFS root) and 
the LED behavior doesn't match to the LED labels anymore.

    Now, about the ways to deal with this issue that I see.
    First I thought about using PHY platform fixup mechanism which is already 
hooked up to a PHY reset in phy_mii_ioctl() (the code is somewhat naive though 
as it doesn't wait for the reset completion before calling fixups and the 
driver's config_init() method). All I needed is calling fixups after direct 
PHY reset done via phy_write() but I didn't want to add a hook there due to 
that reset completion wait loop that was obviously necessary (this function is 
simple *inline*), so went for a simplistic local phy_scan_fixups() call (that 
function was helpfully already exported although not called by anyone except 
phylib itself) after PHY reset in the 'sh_eth' driver which you saw in my 
patch and which you didn't like; I've also coded the PHY platform fixup which 
got successfully merged via arch/arm/mach-shmobile/ subtree at that time. Your 
argument was that the driver is going against the control flow provided by 
phylib (it's not alone doing PHY reset, I should note) and so you wanted me to 
embed everything into phylib control flow. I replied that I fear the 
unexpected consequencies of doing compulsory PHY reset for every driver using 
phylib (that's how I understood your idea at least)... then, after a long 
pause, I suggested to code phy_reset() function within phylib which the 
drivers that currently do reset their PHYs could use instead of open coding 
the reset bit polling loop, etc. and which would include a hook for the 
platform fixups and PHY driver config_init() call. I'd still like to hear your 
opinion on this approach -- which is less invasive.
    What I can also do in this case is again ignore ETH_LINK signal in the 
'sh_eth' driver and stop caring about the LED functions matching to what's 
designed for the boards. This doesn't need PHY platform fixup or any change in 
phylib and Ethernet driver.
    What I also can do is stop resetting PHY in the 'sh_eth' driver (only 
getting it out of sleep for which the reset doesn't seem necessary), although 
we'd still need the PHY platform fixup for the PHY resets done via 
phy_mii_ioctl()... I'm not so much familiar with the driver to be sure it 
won't hurt (the driver is used on e.g. some SH platforms I don't get any 
access to) and I couldn't get any useful input from the driver's primary 
author -- but perhaps it's really not necessary. What do you think?

WBR, Sergei


^ permalink raw reply

* Re: [PATCH v3] net: Do not include padding in TCP GRO checksum
From: Herbert Xu @ 2013-11-16  0:47 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: davem, netdev, edumazet, herbert
In-Reply-To: <20131115225856.6988.69733.stgit@ahduyck-fpga.jf.intel.com>

On Fri, Nov 15, 2013 at 03:00:34PM -0800, Alexander Duyck wrote:
> In some recent tests I found the TCP checksum was being treated as valid
> for certain frames with padding on them.  On closer inspection I found the
> issue was that GRO was using the skb->len instead of the length recorded in
> the IP/IPv6 header to determine the number of bytes to checksum.  As such
> padded frames that actually had invalid checksums generated by adding the
> padding to the checksum were being incorrectly tagged as valid.
> 
> This change corrects that by using the tot_len from IPv4 headers and the
> payload_len from IPv6 headers to compute the total number of bytes to be
> included in the checksum.
> 
> To address the fact that skb->csum is invalid when a padded frame is
> received I have updated the code to fall though to the CHECKSUM_NONE path
> for CHECKSUM_COMPLETE frames that contain padding.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Nack.  This is needlessly complex.  As I said before, please
just do a pskb_trim_rcsum in inet_gro_receive and its IPv6
counterpart and this should all just work.

Also, if the padding occurs on every single packet or a fairly
large amount of packets then you should consider improving
pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* Re: [PATCH v3] net: Do not include padding in TCP GRO checksum
From: David Miller @ 2013-11-16  1:34 UTC (permalink / raw)
  To: herbert; +Cc: alexander.h.duyck, netdev, edumazet, herbert
In-Reply-To: <20131116004738.GA1491@gondor.apana.org.au>

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 16 Nov 2013 08:47:38 +0800

> Also, if the padding occurs on every single packet or a fairly
> large amount of packets then you should consider improving
> pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.

It should be as easy as this?  (this is actually just a 3 line change,
it looks like more because I'm moving pskb_trim_rcsum() later in the
file to where skb_checksum() is in scope).

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 215b5ea..bec1cc7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2263,24 +2263,6 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
 
-/**
- *	pskb_trim_rcsum - trim received skb and update checksum
- *	@skb: buffer to trim
- *	@len: new length
- *
- *	This is exactly the same as pskb_trim except that it ensures the
- *	checksum of received packets are still valid after the operation.
- */
-
-static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
-{
-	if (likely(len >= skb->len))
-		return 0;
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->ip_summed = CHECKSUM_NONE;
-	return __pskb_trim(skb, len);
-}
-
 #define skb_queue_walk(queue, skb) \
 		for (skb = (queue)->next;					\
 		     skb != (struct sk_buff *)(queue);				\
@@ -2378,6 +2360,27 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
 
+/**
+ *	pskb_trim_rcsum - trim received skb and update checksum
+ *	@skb: buffer to trim
+ *	@len: new length
+ *
+ *	This is exactly the same as pskb_trim except that it ensures the
+ *	checksum of received packets are still valid after the operation.
+ */
+
+static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
+{
+	if (likely(len >= skb->len))
+		return 0;
+	if (skb->ip_summed == CHECKSUM_COMPLETE) {
+		__wsum adj = skb_checksum(skb, len, skb->len - len, 0);
+
+		skb->csum = csum_sub(skb->csum, adj);
+	}
+	return __pskb_trim(skb, len);
+}
+
 static inline void *skb_header_pointer(const struct sk_buff *skb, int offset,
 				       int len, void *buffer)
 {

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox