* Re: [PATCH net] r8152: Fix broken RX checksums.
From: Mark Lord @ 2016-11-04 13:50 UTC (permalink / raw)
To: Hayes Wang, David Miller
Cc: nic_swsd, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ddbc14e2-dfb5-01f2-0d5b-5b409a01160f@pobox.com>
Yeah, the device or driver is definitely getting confused with rx_desc structures.
I added code to check for unlikely rx_desc values, and it found this for starters:
rx_desc: 00480801 00480401 00480001 0048fc00 0048f800 0048f400 pkt_len=2045
rx_data: 00 f0 48 00 00 ec 48 00 00 e8 48 00 00 e4 48 00 00 e0 48 00 00 dc 48 00 00 d8 48 00 00 d4 48 00
rx_data: 00 d0 48 00 00 cc 48 00 00 c8 48 00 00 c4 48 00 00 c0 48 00 00 bc 48 00 00 b8 48 00 00 b4 48 00
rx_data: 00 b0 48 00 00 ac 48 00 00 01 00 00 81 ed 00 00 00 01 00 00 00 00 00 00 00 00 00 02 4d ac 00 00
rx_data: 10 00 ff ff ff ff 00 00 01 28 83 d6 ff 6d 00 20 25 b1 58 1b 68 ff 00 05 20 01 56 41 17 35 00 00
...
The MTU/MRU on this link is the standard 1500 bytes, so a pkt_len of 2045 isn't valid here.
And the rx_desc values look an awful lot like the rx_data values that follow it.
There's definitely more broken here than just TCP RX checksums.
-ml
^ permalink raw reply
* Re: [net-next PATCH 0/7] stmmac: dwmac-sti refactor+cleanup
From: Giuseppe CAVALLARO @ 2016-11-04 13:49 UTC (permalink / raw)
To: Joachim Eastwood, davem; +Cc: alexandre.torgue, netdev
In-Reply-To: <20161030200507.851-1-manabian@gmail.com>
Hello Joachim.
I have tested the patches on STiH390 with GMAC4 and the driver is ok.
So you can add my Acked-by/Tested-by in the V2.
I just ask you, when rename the sti_dwmac_init in sti_dwmac_set_phy_mode
to use another name: sti_dwmac_set_mode could be good, IMO.
In fact, this function is not strictly related to the PHY and the
system config registers could touch different MAC/MII settings.
Regards
Peppe
On 10/30/2016 9:05 PM, Joachim Eastwood wrote:
> This patch set aims to remove the init/exit callbacks from the
> dwmac-sti driver and instead use standard PM callbacks. Doing this
> will also allow us to cleanup the driver.
>
> Eventually the init/exit callbacks will be deprecated and removed
> from all drivers dwmac-* except for dwmac-generic. Drivers will be
> refactored to use standard PM and remove callbacks.
>
> Note that this patch set has only been test compiled and no functional
> change is intended.
>
> Joachim Eastwood (7):
> stmmac: dwmac-sti: remove useless of_node check
> stmmac: dwmac-sti: remove clk NULL checks
> stmmac: dwmac-sti: add PM ops and resume function
> stmmac: dwmac-sti: move st,gmac_en parsing to sti_dwmac_parse_data
> stmmac: dwmac-sti: move clk_prepare_enable out of init and add error handling
> stmmac: dwmac-sti: clean up and rename sti_dwmac_init
> stmmac: dwmac-sti: remove unused priv dev member
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 86 ++++++++++++++++---------
> 1 file changed, 57 insertions(+), 29 deletions(-)
>
^ permalink raw reply
* Re: [PATCH net v3] ipv4: allow local fragmentation in ip_finish_output_gso()
From: Lance Richardson @ 2016-11-04 13:49 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: Hannes Frederic Sowa, fw, netdev, jtluka
In-Reply-To: <20161104114014.3dbb8684@halley>
> From: "Shmulik Ladkani" <shmulik.ladkani@gmail.com>
> To: "Hannes Frederic Sowa" <hannes@stressinduktion.org>
> Cc: "Lance Richardson" <lrichard@redhat.com>, fw@strlen.de, netdev@vger.kernel.org, jtluka@redhat.com
> Sent: Friday, November 4, 2016 5:40:14 AM
> Subject: Re: [PATCH net v3] ipv4: allow local fragmentation in ip_finish_output_gso()
>
> On Thu, 3 Nov 2016 22:34:34 +0100 Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Correct, but we should maybe redefine the code a bit. From my
> > understanding we can now create an ICMP storm in case every fragment gets.
>
> Yes, you are right.
>
> Each segment gets into ip_fragment, and due to outer DF being set,
> ICMP_FRAG_NEEDED is sent per segment.
>
> BTW, suppose GRO is off, and sender actually did send a burst of
> (non-gso) packets with outer DF set, and each was tunnel encapsulated,
> resulting in oversized frames.
>
> Would'nt the stack just send the ICMP_FRAG_NEEDED per encapsulated
> frame?
>
> If so, then the GRO behaviour is aligned, and there's nothing to fix.
>
Agree.
> Best,
> Shmulik
>
^ permalink raw reply
* Re: [PATCH net v3] ipv4: allow local fragmentation in ip_finish_output_gso()
From: Lance Richardson @ 2016-11-04 13:48 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: netdev, fw, jtluka, hannes
In-Reply-To: <20161104112409.6277b7b0@halley>
> From: "Shmulik Ladkani" <shmulik.ladkani@gmail.com>
> To: "Lance Richardson" <lrichard@redhat.com>
> Cc: netdev@vger.kernel.org, fw@strlen.de, jtluka@redhat.com, hannes@stressinduktion.org
> Sent: Friday, November 4, 2016 5:24:09 AM
> Subject: Re: [PATCH net v3] ipv4: allow local fragmentation in ip_finish_output_gso()
>
> Hi,
>
> On Thu, 3 Nov 2016 09:06:27 -0400 (EDT) Lance Richardson
> <lrichard@redhat.com> wrote:
> > I'm not sure what could be added that would help, was there something
> > specific you had in mind?
>
> How about something like this (preliminary, feel free to massage):
>
> @@ -248,10 +248,16 @@ static int ip_finish_output_gso(struct net *net, struct
> sock *sk,
>
> /* Slowpath - GSO segment length is exceeding the dst MTU.
> *
> - * This can happen in two cases:
> - * 1) TCP GRO packet, DF bit not set
> - * 2) skb arrived via virtio-net, we thus get TSO/GSO skbs directly
> - * from host network stack.
> + * This can happen in several cases:
> + * - Forwarding of TCP GRO packet, DF bit not set
> + * - Forwarding of skb arrived in a virtualization environment (from
> + * virtio-net/vhost/tap) with TSO/GSO size set by other's network
> + * stack
> + * - Local GSO skb xmitted on an NETIF_F_TSO tunnel stacked over an
> + * interface with a smaller mtu
> + * - Arriving GRO skb (or GSO skb in a virtualized env) that gets L2
> + * bridged to a NETIF_F_TSO tunnel stacked over an interface with an
> + * insufficent mtu
> */
> features = netif_skb_features(skb);
> BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET);
>
Thanks, that looks good to me. I can send a follow-up patch with this change,
if you like (there seems to be agreement that the original patch is OK).
Lance
^ permalink raw reply
* Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver
From: Allan W. Nielsen @ 2016-11-04 13:42 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104122746.GK13959@lunn.ch>
On 04/11/16 13:27, Andrew Lunn wrote:
> > + } else if (count) {
> > + /* Downshift count is either 2,3,4 or 5 */
> > + count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
>
> Ah, now i see why + 2. But this means it never does what you ask it to
> do. It would be better to round up < 2 to 2, and leave all the others
> as is.
Not sure I understand what you mean...
If the user configure "count == 1", then you want that to be rounded up to
"count == 2", because the HW does not support a count of 1???
If the user configure count to 6, 7, 8 etc. would you also like to round it down
to 5?
I'm okay with that but not sure it is want you mean... (and it will eliminate
your comment on ERANGE - because all values will be accepted, they are just
rounded to "closet" supported value).
(The other comments is understood, and we will get them fixed).
/Allan
^ permalink raw reply
* Re: Ethernet not working on a different SoC with same eth HW
From: Måns Rullgård @ 2016-11-04 13:40 UTC (permalink / raw)
To: Mason
Cc: netdev, Andrew Lunn, Florian Fainelli, Timur Tabi,
Sergei Shtylyov, Zefir Kurtisi, Martin Blumenstingl,
Uwe Kleine-Konig, Daniel Mack, Sebastian Frias
In-Reply-To: <581C8691.2060306@free.fr>
Mason <slash.tmp@free.fr> writes:
> On 31/10/2016 17:28, Mason wrote:
>
>> On 31/10/2016 16:53, Andrew Lunn wrote:
>>
>>>> I'll add a log for the request_irq call.
>>>
>>> And take a look at /proc/interrupts
>>
>> You're right, there does seem to be something wrong with the interrupts.
>
> Having fixed that, I'm still unable to ping a box on the same
> ethernet segment... Still investigating.
>
> I think I may have spotted a potential issue in the Atheros driver.
>
> if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> ret = at803x_enable_rx_delay(phydev);
> if (ret < 0)
> return ret;
> }
>
> Looking at this code, one might believe that "rgmii rx clock delay"
> is only enabled when the user requests it (through DT).
>
> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
> cf. PDF page 48
>
> *However* this bit is set to 1 at reset (both HW and SW resets).
> Thus, "rgmii rx clock delay" is always enabled, whether the user
> requests it or not.
>
> Could someone knowledgeable comment on the expected behavior of
> enabling rgmii rx (and tx) clock delay?
Clock delay is sometimes (depending on PCB layout) required to achieve
the correct timing between clock and data signals. The delay can be
applied at the MAC or the PHY. I'd start by finding out what the PCB
design expects or check the signals with a fast oscilloscope if you have
one.
--
Måns Rullgård
^ permalink raw reply
* Re: [PATCH v6 7/7] arm64: dts: NS2: add AMAC ethernet support
From: Sergei Shtylyov @ 2016-11-04 13:31 UTC (permalink / raw)
To: Jon Mason, David Miller, Rob Herring, Mark Rutland,
Florian Fainelli
Cc: rafal-g1n6cQUeyibVItvQsEIGlw,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1478236262-3351-8-git-send-email-jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Hello.
On 11/4/2016 8:11 AM, Jon Mason wrote:
> Add support for the AMAC ethernet to the Broadcom Northstar2 SoC device
> tree
>
> Signed-off-by: Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
> arch/arm64/boot/dts/broadcom/ns2-svk.dts | 5 +++++
> arch/arm64/boot/dts/broadcom/ns2.dtsi | 12 ++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> index b09f3bc..c4d5442 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> @@ -56,6 +56,10 @@
> };
> };
>
> +&enet {
> + status = "ok";
The spec dictates it should be "okay" (although "ok" is also recognized).
> +};
> +
> &pci_phy0 {
> status = "ok";
> };
> @@ -174,6 +178,7 @@
> &mdio_mux_iproc {
> mdio@10 {
> gphy0: eth-phy@10 {
> + enet-phy-lane-swap;
> reg = <0x10>;
> };
> };
[...]
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE
From: Allan W. Nielsen @ 2016-11-04 13:31 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104121332.GJ13959@lunn.ch>
On 04/11/16 13:13, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Adding get_tunable/set_tunable function pointer to the phy_driver
> > structure, and uses these function pointers to implement the
> > ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.
> For consistency, it would also be nice to add code in
> __ethtool_get_strings().
Okay, we add add that, and I assume you also want ethtool to use it when the
"-k|--show-features" options is invoked?
Are there other cases where this should be used?
/Allan
^ permalink raw reply
* Re: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
From: Neil Horman @ 2016-11-04 13:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, Vlad Yasevich, syzkaller, kcc, glider,
edumazet, dvyukov, andreyknvl
In-Reply-To: <9df38dcd0323ad92386eb6851a60dc128dd00b4e.1478199530.git.marcelo.leitner@gmail.com>
On Thu, Nov 03, 2016 at 05:03:41PM -0200, Marcelo Ricardo Leitner wrote:
> sctp_wait_for_connect() currently already holds the asoc to keep it
> alive during the sleep, in case another thread release it. But Andrey
> Konovalov and Dmitry Vyukov reported an use-after-free in such
> situation.
>
> Problem is that __sctp_connect() doesn't get a ref on the asoc and will
> do a read on the asoc after calling sctp_wait_for_connect(), but by then
> another thread may have closed it and the _put on sctp_wait_for_connect
> will actually release it, causing the use-after-free.
>
> Fix is, instead of doing the read after waiting for the connect, do it
> before so, and avoid this issue as the socket is still locked by then.
> There should be no issue on returning the asoc id in case of failure as
> the application shouldn't trust on that number in such situations
> anyway.
>
> This issue doesn't exist in sctp_sendmsg() path.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438aa9b6dbdad93e70759071a4d6789..be1d9bb98230c9d77f676949db773b2dacd801a4 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
>
> /* Don't free association on exit. */
> asoc = NULL;
> --
> 2.7.4
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ethtool 2/2] Ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE and PHY downshift
From: Allan W. Nielsen @ 2016-11-04 13:28 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104115937.GH13959@lunn.ch>
On 04/11/16 12:59, Andrew Lunn wrote:
> > +++ b/ethtool.8.in
> > @@ -340,6 +340,14 @@ ethtool \- query or control network driver and hardware settings
> > .B2 tx-lpi on off
> > .BN tx-timer
> > .BN advertise
> > +.HP
> > +.B ethtool \-\-set\-phy\-tunable
> > +.I devname
> > +.B3 downshift on off N
> > +.HP
>
> I don't think there is any other option which is on|off|N. The general
> pattern would be
>
> --set-phy-tunable downshift on|off [count N]
>
> With count being optional.
>
> This also allows avoiding the ambiguity of what
>
> --set-phy-tunable downshift 0
>
> means. To me, that means downshift after 0 retries. But it actually
> seems to mean never downshift. You can then error out when somebody
> does
>
> --set-phy-tunable downshift on count 0
> or
> --set-phy-tunable downshift off count 42
Okay, I do not have any strong feelings about the syntax here.
We could not find any other signature which had the same need, so we have been
looking for things that was "close" to...
What is being proposed is similar to "-s [ mdix auto|on|off ]".
What you request is similar to "-e|--eeprom-dump [ raw on|off ] [ offset N ]".
So, unless there are other comments, then we will change this in the next
version.
> Please also add a few sentences about what downshift is, and what N
> means. Is it tries, or retries?
Sure.
We will wait a few days to see what other comments we get.
Thanks for reviewing.
/Allan
^ permalink raw reply
* Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function
From: Alan Stern @ 2016-11-04 13:26 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: oneukum, netdev, linux-usb, linux-kernel
In-Reply-To: <20161104095735.21071-1-kai.heng.feng@canonical.com>
On Fri, 4 Nov 2016, Kai-Heng Feng wrote:
> Sometimes cdc_mbim failed to probe if runtime pm is enabled:
> [ 9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
>
> This can be solved by increase its pm usage counter.
This should not be needed. The USB core increments the PM usage
counter of a device before probing its interfaces.
Alan Stern
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/net/usb/usbnet.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index d5071e3..f77b4bf 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1674,12 +1674,15 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
> net->ethtool_ops = &usbnet_ethtool_ops;
>
> + if (usb_autopm_get_interface(dev->intf) < 0)
> + goto out1;
> +
> // allow device-specific bind/init procedures
> // NOTE net->name still not usable ...
> if (info->bind) {
> status = info->bind (dev, udev);
> if (status < 0)
> - goto out1;
> + goto out2;
>
> // heuristic: "usb%d" for links we know are two-host,
> // else "eth%d" when there's reasonable doubt. userspace
> @@ -1772,6 +1775,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> out3:
> if (info->unbind)
> info->unbind (dev, udev);
> +out2:
> + usb_autopm_put_interface(dev->intf);
> out1:
> /* subdrivers must undo all they did in bind() if they
> * fail it, but we may fail later and a deferred kevent
>
^ permalink raw reply
* Re: net/sctp: use-after-free in __sctp_connect
From: Marcelo Ricardo Leitner @ 2016-11-04 13:03 UTC (permalink / raw)
To: Neil Horman
Cc: Andrey Konovalov, Vlad Yasevich, David S. Miller, linux-sctp,
netdev, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
Eric Dumazet, Dmitry Vyukov
In-Reply-To: <20161104125958.GA13691@hmsreliant.think-freely.org>
On Fri, Nov 04, 2016 at 08:59:58AM -0400, Neil Horman wrote:
> On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > > <marcelo.leitner@gmail.com> wrote:
> > > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > > >> > <marcelo.leitner@gmail.com> wrote:
> > > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > > >> >>> Hi,
> > > >> >>>
> > > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > > >> >>>
> > > >> >>> ==================================================================
> > > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > > >> >>> ffff88006b1dc610
> > > >> >>
> > > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > > >> >> So far I couldn't identify the reason.
> > > >> >> "Good" to know it's still there, thanks for reporting it.
> > > >>
> > > >> Hi Marcelo,
> > > >>
> > > >
> > > > Hi
> > > >
> > > >> So I've looked at the code.
> > > >> As far as I understand, the problem is a race condition between
> > > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > > >> with sctp_association_put() and returns err = 0.
> > > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > > >> from the freed asoc.
> > > >
> > > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > > looking deeper into this, Andrey.
> > > >
> > > > This fix should do it, can you please try it? I'll post it properly
> > > > if it works.
> > >
> > > Yes, it fixes the issue.
> > >
> > > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > >
> > > Thanks for the fix!
> >
> > Ahm this other fix looks better: do the read before calling
> > sctp_wait_for_connect() as that id won't change in this call and the
> > application shouldn't trust this number if an error is returned, so
> > there should be no issues by returning it in such situation.
> >
> > Can you please confirm this one also works? Thanks!
> >
> > ---8<---
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 6cdc61c21438..be1d9bb98230 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
> >
> > timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
> >
> > - err = sctp_wait_for_connect(asoc, &timeo);
> > - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> > + if (assoc_id)
> > *assoc_id = asoc->assoc_id;
> > + err = sctp_wait_for_connect(asoc, &timeo);
> > + /* Note: the asoc may be freed after the return of
> > + * sctp_wait_for_connect.
> > + */
> >
> > /* Don't free association on exit. */
> > asoc = NULL;
> >
> Agreed, this version looks better. Please repost it with a proper changelog and
> I'll ack it.
> Neil
It already is. :-)
Please look for
Subject: [PATCH net] sctp: assign assoc_id earlier in __sctp_connect
Thanks,
Marcelo
^ permalink raw reply
* Re: Ethernet not working on a different SoC with same eth HW
From: Mason @ 2016-11-04 13:01 UTC (permalink / raw)
To: netdev
Cc: Andrew Lunn, Mans Rullgard, Florian Fainelli, Timur Tabi,
Sergei Shtylyov, Zefir Kurtisi, Martin Blumenstingl,
Uwe Kleine-Konig, Daniel Mack, Sebastian Frias
In-Reply-To: <58177128.8090403@free.fr>
On 31/10/2016 17:28, Mason wrote:
> On 31/10/2016 16:53, Andrew Lunn wrote:
>
>>> I'll add a log for the request_irq call.
>>
>> And take a look at /proc/interrupts
>
> You're right, there does seem to be something wrong with the interrupts.
Having fixed that, I'm still unable to ping a box on the same
ethernet segment... Still investigating.
I think I may have spotted a potential issue in the Atheros driver.
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_rx_delay(phydev);
if (ret < 0)
return ret;
}
Looking at this code, one might believe that "rgmii rx clock delay"
is only enabled when the user requests it (through DT).
http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
cf. PDF page 48
*However* this bit is set to 1 at reset (both HW and SW resets).
Thus, "rgmii rx clock delay" is always enabled, whether the user
requests it or not.
Could someone knowledgeable comment on the expected behavior of
enabling rgmii rx (and tx) clock delay?
https://en.wikipedia.org/wiki/Media-independent_interface#Reduced_gigabit_media-independent_interface
Regards.
^ permalink raw reply
* Re: net/sctp: use-after-free in __sctp_connect
From: Neil Horman @ 2016-11-04 12:59 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Andrey Konovalov, Vlad Yasevich, David S. Miller, linux-sctp,
netdev, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko,
Eric Dumazet, Dmitry Vyukov
In-Reply-To: <20161103183533.GH8514@localhost.localdomain>
On Thu, Nov 03, 2016 at 04:35:33PM -0200, Marcelo Ricardo Leitner wrote:
> On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:
> > On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> > > On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:
> > >> On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > >> > On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner
> > >> > <marcelo.leitner@gmail.com> wrote:
> > >> >> On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:
> > >> >>> Hi,
> > >> >>>
> > >> >>> I've got the following error report while running the syzkaller fuzzer:
> > >> >>>
> > >> >>> ==================================================================
> > >> >>> BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr
> > >> >>> ffff88006b1dc610
> > >> >>
> > >> >> Seems this is the same that Dmitry Vyukov had reported back in Jan 13th.
> > >> >> So far I couldn't identify the reason.
> > >> >> "Good" to know it's still there, thanks for reporting it.
> > >>
> > >> Hi Marcelo,
> > >>
> > >
> > > Hi
> > >
> > >> So I've looked at the code.
> > >> As far as I understand, the problem is a race condition between
> > >> setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket.
> > >> setsockopt() calls sctp_wait_for_connect(), which exits the for loop
> > >> on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc
> > >> with sctp_association_put() and returns err = 0.
> > >> Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id
> > >> from the freed asoc.
> > >
> > > Suddenly this seems familiar. Your description makes sense, thanks for
> > > looking deeper into this, Andrey.
> > >
> > > This fix should do it, can you please try it? I'll post it properly
> > > if it works.
> >
> > Yes, it fixes the issue.
> >
> > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> >
> > Thanks for the fix!
>
> Ahm this other fix looks better: do the read before calling
> sctp_wait_for_connect() as that id won't change in this call and the
> application shouldn't trust this number if an error is returned, so
> there should be no issues by returning it in such situation.
>
> Can you please confirm this one also works? Thanks!
>
> ---8<---
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6cdc61c21438..be1d9bb98230 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1214,9 +1214,12 @@ static int __sctp_connect(struct sock *sk,
>
> timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);
>
> - err = sctp_wait_for_connect(asoc, &timeo);
> - if ((err == 0 || err == -EINPROGRESS) && assoc_id)
> + if (assoc_id)
> *assoc_id = asoc->assoc_id;
> + err = sctp_wait_for_connect(asoc, &timeo);
> + /* Note: the asoc may be freed after the return of
> + * sctp_wait_for_connect.
> + */
>
> /* Don't free association on exit. */
> asoc = NULL;
>
Agreed, this version looks better. Please repost it with a proper changelog and
I'll ack it.
Neil
^ permalink raw reply
* Re: [net-next PATCH 3/3] qdisc: catch misconfig of attaching qdisc to tx_queue_len zero device
From: Sergei Shtylyov @ 2016-11-04 12:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev
Cc: Phil Sutter, Robert Olsson, Jamal Hadi Salim
In-Reply-To: <20161103135611.28737.39840.stgit@firesoul>
Hello.
On 11/3/2016 4:56 PM, Jesper Dangaard Brouer wrote:
> It is a clear misconfiguration to attach a qdisc to a device with
> tx_queue_len zero, because some qdisc's (namely, pfifo, bfifo, gred,
> htb, plug and sfb) inherit/copy this value as their queue length.
>
> Why should the kernel catch such a misconfiguration? Because prior to
> introducing the IFF_NO_QUEUE device flag, userspace found a loophole
> in the qdisc config system that allowed them to achieve the equivalent
> of IFF_NO_QUEUE, which is to remove the qdisc code path entirely from
> a device. The loophole on older kernels is setting tx_queue_len=0,
> *prior* to device qdisc init (the config time is significant, simply
> setting tx_queue_len=0 doesn't trigger the loophole).
>
> This loophole is currently used by Docker[1] to get better performance
> and scalability out of the veth device. The Docker developers were
> warned[1] that they needed to adjust the tx_queue_len if ever
> attaching a qdisc. The OpenShift project didn't remember this warning
> and attached a qdisc, this were caught and fixed in[2].
>
> [1] https://github.com/docker/libcontainer/pull/193
> [2] https://github.com/openshift/origin/pull/11126
>
> Instead of fixing every userspace program that used this loophole, and
> forgot to reset the tx_queue_len, prior to attaching a qdisc. Let's
> catch the misconfiguration on the kernel side.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> net/sched/sch_api.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 206dc24add3a..f337f1bdd1d4 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>
> sch->handle = handle;
>
> + /* This exist to keep backward compatible with a userspace
"Exists" and "compatibility".
> + * loophole, what allowed userspace to get IFF_NO_QUEUE
> + * facility on older kernels by setting tx_queue_len=0 (prior
> + * to qdisc init), and then forgot to reinit tx_queue_len
> + * before again attaching a qdisc.
> + */
> + if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
> + dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> + netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
> + }
> +
> if (!ops->init || (err = ops->init(sch, tca[TCA_OPTIONS])) == 0) {
> if (qdisc_is_percpu_stats(sch)) {
> sch->cpu_bstats =
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] udp: do fwd memory scheduling on dequeue
From: Eric Dumazet @ 2016-11-04 12:38 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, David S. Miller, Eric Dumazet, Hannes Frederic Sowa,
linux-nfs
In-Reply-To: <762f142043b48daf282316dd88ff099fbd9729e4.1478254512.git.pabeni@redhat.com>
On Fri, 2016-11-04 at 11:28 +0100, Paolo Abeni wrote:
> A new argument is added to __skb_recv_datagram to provide
> an explicit skb destructor, invoked under the receive queue
> lock.
> The UDP protocol uses such argument to perform memory
> reclaiming on dequeue, so that the UDP protocol does not
> set anymore skb->desctructor.
> Instead explicit memory reclaiming is performed at close() time and
> when skbs are removed from the receive queue.
> The in kernel UDP protocol users now need to call a
> skb_recv_udp() variant instead of skb_recv_datagram() to
> properly perform memory accounting on dequeue.
>
> Overall, this allows acquiring only once the receive queue
> lock on dequeue.
>
> Tested using pktgen with random src port, 64 bytes packet,
> wire-speed on a 10G link as sender and udp_sink as the receiver,
> using an l4 tuple rxhash to stress the contention, and one or more
> udp_sink instances with reuseport.
>
> nr sinks vanilla patched
> 1 440 560
> 3 2150 2300
> 6 3650 3800
> 9 4450 4600
> 12 6250 6450
>
> v1 -> v2:
> - do rmem and allocated memory scheduling under the receive lock
> - do bulk scheduling in first_packet_length() and in udp_destruct_sock()
> - avoid the typdef for the dequeue callback
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> @Eric, please add your signed off by when you feel comfortable with the patch
> as you authored some of the code
SGTM, thanks Paolo and Hannes !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] net/sock: add an explicit sk argument for ip_cmsg_recv_offset()
From: Eric Dumazet @ 2016-11-04 12:36 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller, Eric Dumazet,
Hannes Frederic Sowa, linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1b4e5292a0c15494515423245e8eeb7adf2ebcea.1478254512.git.pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, 2016-11-04 at 11:28 +0100, Paolo Abeni wrote:
> So that we can use it even after orphaining the skbuff.
>
> Suggested-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> @Eric, please add your signed off by when you feel comfortable with the patch
> as you basically authored it
> -void ip_cmsg_recv_offset(struct msghdr *msg, struct sk_buff *skb,
> - int tlen, int offset)
> +void ip_cmsg_recv_offset(struct msghdr *msg, struct sock *sk,
> + struct sk_buff *skb, int tlen, int offset)
> {
> - struct inet_sock *inet = inet_sk(skb->sk);
> + struct inet_sock *inet = inet_sk(sk);
> unsigned int flags = inet->cmsg_flags;
My final version had :
void ip_cmsg_recv_offset(struct msghdr *msg, const struct sock *sk,
struct sk_buff *skb, int tlen, int offset)
{
unsigned int flags = inet_sk(sk)->cmsg_flags;
(Ie not using "struct inet_sock *inet = ...", and a const pointer for
struct sock)
Other than that minor details :
Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Thanks !
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Documentation: networking: dsa: Update tagging protocols
From: Andrew Lunn @ 2016-11-04 12:31 UTC (permalink / raw)
To: Fabian Mewes
Cc: Jonathan Corbet, David S. Miller, Vivien Didelot,
Florian Fainelli, Masanari Iida, linux-doc, linux-kernel, netdev
In-Reply-To: <1478261774-14265-1-git-send-email-architekt@coding4coffee.org>
On Fri, Nov 04, 2016 at 01:16:14PM +0100, Fabian Mewes wrote:
> Add Qualcomm QCA tagging introduced in cafdc45c9 to the
> list of supported protocols.
>
> Signed-off-by: Fabian Mewes <architekt@coding4coffee.org>
Thanks
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
From: Andrew Lunn @ 2016-11-04 12:29 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104121841.GA5676@microsemi.com>
> It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
> is done.
Yes, i know. I'm wondering about cult cargo programming...
> The thinking was that we did not want an "ID" of zero do to anything - because
> that could mean the programmer had forgot to set the field...
Seems reasonable. Leave it as is.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver
From: Andrew Lunn @ 2016-11-04 12:27 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255742-25693-6-git-send-email-allan.nielsen@microsemi.com>
Hi Allan
> +static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
> +{
> + int rc;
> + u16 reg_val;
> +
> + mutex_lock(&phydev->lock);
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> + if (rc != 0)
> + goto out_unlock;
> +
> + reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> + reg_val &= DOWNSHIFT_CNTL_MASK;
> + if (!(reg_val & DOWNSHIFT_EN))
> + *count = 0;
DOWNSHIFT_DEV_DISABLE
> + else
> + *count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
>
> + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> + mutex_unlock(&phydev->lock);
> +
> + return rc;
> +}
> +
> +static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
> +{
> + int rc;
> + u16 reg_val;
> +
> + if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
> + /* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
> + count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> + } else if (count > DOWNSHIFT_COUNT_MAX || count == 1) {
> + phydev_err(phydev, "Invalid downshift count\n");
Maybe include a hint what is valid?
> + return -EINVAL;
ERANGE? I don't know error codes too well, so this needs verifying.
> + } else if (count) {
> + /* Downshift count is either 2,3,4 or 5 */
> + count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
Ah, now i see why + 2. But this means it never does what you ask it to
do. It would be better to round up < 2 to 2, and leave all the others
as is.
> +static int vsc85xx_get_tunable(struct phy_device *phydev,
> + struct ethtool_tunable *tuna, void *data)
> +{
> + switch (tuna->id) {
> + case ETHTOOL_PHY_DOWNSHIFT:
> + return vsc85xx_downshift_get(phydev, (u8 *)data);
> + default:
> + phydev_err(phydev, "Unsupported PHY tunable id\n");
> + return -EINVAL;
This is not really a error you should complain about. There could be
many tunables, and some your hardware cannot support. So return
-ENOSUPP and no phydev_err().
> + }
> +}
> +
> +static int vsc85xx_set_tunable(struct phy_device *phydev,
> + struct ethtool_tunable *tuna,
> + const void *data)
> +{
> + switch (tuna->id) {
> + case ETHTOOL_PHY_DOWNSHIFT:
> + return vsc85xx_downshift_set(phydev, *(u8 *)data);
> + default:
> + phydev_err(phydev, "Unsupported PHY tunable id\n");
> + return -EINVAL;
Same as above.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
From: Allan W. Nielsen @ 2016-11-04 12:18 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <20161104120343.GI13959@lunn.ch>
Hi,
On 04/11/16 13:03, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:38AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Defines a generic API to get/set phy tunables. The API is using the
> > existing ethtool_tunable/tunable_type_id types which is already being used
> > for mac level tunables.
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> > Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> > ---
> > include/uapi/linux/ethtool.h | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 8e54723..fd0bd36 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -248,6 +248,10 @@ struct ethtool_tunable {
> > void *data[0];
> > };
> >
> > +enum phy_tunable_id {
> > + ETHTOOL_PHY_ID_UNSPEC,
> > +};
>
> Do you have any idea what this is for? A grep for
> ETHTOOL_TUNABLE_UNSPEC does not turn up anything.
It is not used...
It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
is done.
The thinking was that we did not want an "ID" of zero do to anything - because
that could mean the programmer had forgot to set the field...
I have on strong feelings about this, please let us know if you would like this
done in an other way.
/Allan
^ permalink raw reply
* [PATCH] Documentation: networking: dsa: Update tagging protocols
From: Fabian Mewes @ 2016-11-04 12:16 UTC (permalink / raw)
To: Jonathan Corbet
Cc: David S. Miller, Vivien Didelot, Florian Fainelli, Andrew Lunn,
Masanari Iida, linux-doc, linux-kernel, netdev, Fabian Mewes
Add Qualcomm QCA tagging introduced in cafdc45c9 to the
list of supported protocols.
Signed-off-by: Fabian Mewes <architekt@coding4coffee.org>
---
Documentation/networking/dsa/dsa.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index 6d6c07c..63912ef3 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -67,13 +67,14 @@ Note that DSA does not currently create network interfaces for the "cpu" and
Switch tagging protocols
------------------------
-DSA currently supports 4 different tagging protocols, and a tag-less mode as
+DSA currently supports 5 different tagging protocols, and a tag-less mode as
well. The different protocols are implemented in:
net/dsa/tag_trailer.c: Marvell's 4 trailer tag mode (legacy)
net/dsa/tag_dsa.c: Marvell's original DSA tag
net/dsa/tag_edsa.c: Marvell's enhanced DSA tag
net/dsa/tag_brcm.c: Broadcom's 4 bytes tag
+net/dsa/tag_qca.c: Qualcomm's 2 bytes tag
The exact format of the tag protocol is vendor specific, but in general, they
all contain something which:
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net] r8152: Fix broken RX checksums.
From: Mark Lord @ 2016-11-04 12:13 UTC (permalink / raw)
To: Hayes Wang, David Miller
Cc: nic_swsd, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <008b7d7d-74af-0341-0c16-40aafde5a223@pobox.com>
On 16-11-02 02:29 PM, Mark Lord wrote:
>
> I have poked at it some more, and thus far it appears that it is
> only necessary to disable TCP rx checksums. The system doesn't crash
> when only IP/UDP checksums are enabled, but does when TCP checksums are on.
>
> This happens regardless of whether RX_AGG is disabled or enabled,
> and increasing/decreasing the number of RX URBs (RTL8152_MAX_RX)
> doesn't seem to affect it.
>
..
I noticed that BIT(20) was not defined as anything for "opts3",
and so I added a line to rx_csum to check whether or not that bit
was ever set.
It triggered after a few thousand reset/reboot cycles with opts3
having the rather dubious looking value of an ASCII string: 0x5c7d7852.
So to me, it appears that the rx_desc's are getting mixed-up with data somewhere,
and when using hardware TCP checksums this doesn't get caught. So perhaps the
checksums themselves are fine, but there's another bug (driver or hardware?)
that sneaks through when not doing software checksums.
^ permalink raw reply
* Re: [PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE
From: Andrew Lunn @ 2016-11-04 12:13 UTC (permalink / raw)
To: Allan W. Nielsen; +Cc: netdev, f.fainelli, raju.lakkaraju, cphealy, robh
In-Reply-To: <1478255742-25693-3-git-send-email-allan.nielsen@microsemi.com>
On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
>
> Adding get_tunable/set_tunable function pointer to the phy_driver
> structure, and uses these function pointers to implement the
> ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.
Hi Allan
For consistency, it would also be nice to add code in
__ethtool_get_strings().
Andrew
^ permalink raw reply
* Re: [net-next PATCH 3/3] qdisc: catch misconfig of attaching qdisc to tx_queue_len zero device
From: Jesper Dangaard Brouer @ 2016-11-04 12:09 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev, Robert Olsson, Jamal Hadi Salim, brouer
In-Reply-To: <20161104105913.GN5640@orbyte.nwl.cc>
On Fri, 4 Nov 2016 11:59:13 +0100
Phil Sutter <phil@nwl.cc> wrote:
> On Fri, Nov 04, 2016 at 11:10:42AM +0100, Jesper Dangaard Brouer wrote:
> >
> > On Fri, 4 Nov 2016 10:35:26 +0100 Phil Sutter <phil@nwl.cc> wrote:
> >
> > > Hi,
> > >
> > > On Thu, Nov 03, 2016 at 02:56:11PM +0100, Jesper Dangaard Brouer wrote:
> > > [...]
> > > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > > index 206dc24add3a..f337f1bdd1d4 100644
> > > > --- a/net/sched/sch_api.c
> > > > +++ b/net/sched/sch_api.c
> > > > @@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> > > >
> > > > sch->handle = handle;
> > > >
> > > > + /* This exist to keep backward compatible with a userspace
> > > > + * loophole, what allowed userspace to get IFF_NO_QUEUE
> > > > + * facility on older kernels by setting tx_queue_len=0 (prior
> > > > + * to qdisc init), and then forgot to reinit tx_queue_len
> > > > + * before again attaching a qdisc.
> > > > + */
> > > > + if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
> > > > + dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> > > > + netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
> > > > + }
> > >
> > > I wonder why this is limited to IFF_NO_QUEUE devices. Do you think there
> > > is a valid use case for physical ones?
> >
> > Hmmm, I cannot come up with a useful use-case for physical devices, but
> > I cannot see why we should save users that had used the loophole on
> > physical devices, as that is clearly a faulty config to begin with.
> > See net_crit_ratelimited warning here:
> > [1] https://github.com/torvalds/linux/blob/v4.9-rc3/net/core/dev.c#L3403
>
> I really feel like nit-picking again,
Perhaps a follow up patch is better? This patch does solve a real
issue.
> but what differs in between
> loophole users of virtual devices (whose broken scripts stopped working)
> and loophole users of physical devices (whose broken scripts stopped
> working as well)?
There is a difference. We basically closed the loophole config, but
fixed that qdisc can be attached to virtual (IFF_NO_QUEUE) devices,
without needing to adjusting tx_queue_len.
Thus, running a loophole-script have no-effect, but for IFF_NO_QUEUE
devices (veth specifically) it looks like it had the desired effect,
thus Docker will/can keep doing that, to work with older kernels, and
on newer kernels it just doesn't have any effect.
The remaining problem is that a "loophole-script" leaves the interface
in a broken state with tx_queue_len==0. Which this patch address.
So, why only catch misconfig for IFF_NO_QUEUE devices? Because a
loophole-script on veth brought it into a valid config, thus valid
use-case, while one a physical into a invalid config (hence the
critical warn[1]).
You could (in a followup patch, please) argue that it is a lot simpler,
just to always catch the misconfig of having tx_queue_len==0 when
attaching a qdisc.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox