Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1519592 > unrolled thread

[PATCH net 0/2] r8152: rx patches

Back to article view | Back to linux.kernel

Contents

  [PATCH net 0/2] r8152: rx patches Hayes Wang <hayeswang@realtek.com> - 2016-11-11 08:20 +0100
    [PATCH net 2/2] r8152: rx descriptor check Hayes Wang <hayeswang@realtek.com> - 2016-11-11 08:20 +0100
      Re: [PATCH net 2/2] r8152: rx descriptor check Francois Romieu <romieu@fr.zoreil.com> - 2016-11-11 13:20 +0100
        Re: [PATCH net 2/2] r8152: rx descriptor check Mark Lord <mlord@pobox.com> - 2016-11-12 14:30 +0100
        RE: [PATCH net 2/2] r8152: rx descriptor check Hayes Wang <hayeswang@realtek.com> - 2016-11-14 07:50 +0100
          Re: [PATCH net 2/2] r8152: rx descriptor check Francois Romieu <romieu@fr.zoreil.com> - 2016-11-15 02:20 +0100
            RE: [PATCH net 2/2] r8152: rx descriptor check Hayes Wang <hayeswang@realtek.com> - 2016-11-17 04:10 +0100
      Re: [PATCH net 2/2] r8152: rx descriptor check David Miller <davem@davemloft.net> - 2016-11-13 18:50 +0100
        Re: [PATCH net 2/2] r8152: rx descriptor check Mark Lord <mlord@pobox.com> - 2016-11-13 21:40 +0100
          Re: [PATCH net 2/2] r8152: rx descriptor check Mark Lord <mlord@pobox.com> - 2016-11-13 21:40 +0100
          RE: [PATCH net 2/2] r8152: rx descriptor check Hayes Wang <hayeswang@realtek.com> - 2016-11-14 08:30 +0100
            Re: [PATCH net 2/2] r8152: rx descriptor check David Miller <davem@davemloft.net> - 2016-11-14 18:30 +0100
        RE: [PATCH net 2/2] r8152: rx descriptor check Hayes Wang <hayeswang@realtek.com> - 2016-11-14 08:10 +0100
    [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-11 08:20 +0100
      RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-17 04:40 +0100
        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-17 15:30 +0100
        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-17 15:40 +0100
        RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-18 09:00 +0100
          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-18 13:10 +0100
            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-22 14:20 +0100
            RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-23 05:00 +0100
              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-23 14:50 +0100
                RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-23 16:20 +0100
                  Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-23 20:30 +0100
                    RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 04:30 +0100
                    Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 13:40 +0100
                      RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 14:30 +0100
                        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 17:30 +0100
                          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 17:50 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 18:10 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 18:20 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 18:20 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 19:40 +0100
                                Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 19:50 +0100
                                Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <greg@kroah.com> - 2016-11-24 20:10 +0100
                                  Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <greg@kroah.com> - 2016-11-24 20:20 +0100
                                  Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 20:20 +0100
                                Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Francois Romieu <romieu@fr.zoreil.com> - 2016-11-25 01:30 +0100
                                  Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-25 04:50 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <gregkh@linuxfoundation.org> - 2016-11-24 19:50 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 20:00 +0100
                            RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 07:40 +0100
                              RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 08:00 +0100
                        RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 07:20 +0100
                      Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 17:30 +0100
                RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 13:40 +0100

Page 1 of 3  [1] 2 3  Next page →

#1519592 — [PATCH net 0/2] r8152: rx patches

Let the rx sw checksum available and add some checks for rx desc.

Hayes Wang (2):
  r8152: fix the sw rx checksum is unavailable
  r8152: rx descriptor check

 drivers/net/usb/r8152.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1519594 — [PATCH net 2/2] r8152: rx descriptor check

For some platforms, the data in memory is not the same with the one
from the device. That is, the data of memory is unbelievable. The
check is used to find out this situation.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0e42a78..e766121 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1756,6 +1756,43 @@ static u8 r8152_rx_csum(struct r8152 *tp, struct rx_desc *rx_desc)
 	return checksum;
 }
 
+static int invalid_rx_desc(struct r8152 *tp, struct rx_desc *rx_desc)
+{
+	u32 opts1 = le32_to_cpu(rx_desc->opts1);
+	u32 opts2 = le32_to_cpu(rx_desc->opts2);
+	unsigned int pkt_len = opts1 & RX_LEN_MASK;
+
+	switch (tp->version) {
+	case RTL_VER_01:
+	case RTL_VER_02:
+		if (pkt_len > RTL8152_RMS)
+			return -EIO;
+		break;
+	default:
+		if (pkt_len > RTL8153_RMS)
+			return -EIO;
+		break;
+	}
+
+	switch (opts2 & (RD_IPV4_CS | RD_IPV6_CS)) {
+	case (RD_IPV4_CS | RD_IPV6_CS):
+		return -EIO;
+	case RD_IPV4_CS:
+	case RD_IPV6_CS:
+		switch (opts2 & (RD_UDP_CS | RD_TCP_CS)) {
+		case (RD_UDP_CS | RD_TCP_CS):
+			return -EIO;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int rx_bottom(struct r8152 *tp, int budget)
 {
 	unsigned long flags;
@@ -1812,6 +1849,18 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			unsigned int pkt_len;
 			struct sk_buff *skb;
 
+			if (unlikely(invalid_rx_desc(tp, rx_desc))) {
+				if (net_ratelimit())
+					netif_err(tp, rx_err, netdev,
+						  "Memory unbelievable\n");
+				if (tp->netdev->features & NETIF_F_RXCSUM) {
+					tp->netdev->features &= ~NETIF_F_RXCSUM;
+					netif_err(tp, rx_err, netdev,
+						  "rx checksum off\n");
+				}
+				break;
+			}
+
 			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
 			if (pkt_len < ETH_ZLEN)
 				break;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1519752 — Re: [PATCH net 2/2] r8152: rx descriptor check

Hayes Wang <hayeswang@realtek.com> :
> For some platforms, the data in memory is not the same with the one
> from the device. That is, the data of memory is unbelievable. The
> check is used to find out this situation.

Invalid packet size corrupted receive descriptors in Realtek's device
reminds of CVE-2009-4537.

Is the silicium of both devices different enough to prevent the same
exploit to happen ?

-- 
Ueimor

[toc] | [prev] | [next] | [standalone]

#1520240 — Re: [PATCH net 2/2] r8152: rx descriptor check

On 16-11-11 07:13 AM, Francois Romieu wrote:
> Hayes Wang <hayeswang@realtek.com> :
>> For some platforms, the data in memory is not the same with the one
>> from the device. That is, the data of memory is unbelievable. The
>> check is used to find out this situation.
> 
> Invalid packet size corrupted receive descriptors in Realtek's device
> reminds of CVE-2009-4537.
> 
> Is the silicium of both devices different enough to prevent the same
> exploit to happen ?

I don't know if the hardware can do it, but the existing Linux device
driver regularly attempts to process huge unreal packet sizes here.
I've had to patch it to reject "packets" larger than the configured MRU.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1521353 — RE: [PATCH net 2/2] r8152: rx descriptor check

Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Friday, November 11, 2016 8:13 PM
[...]
> Invalid packet size corrupted receive descriptors in Realtek's device
> reminds of CVE-2009-4537.

Do you mean that the driver would get a packet exceed the size
which is set to RxMaxSize? I check it with our hw engineers.
They don't get any issue about RxMaxSize. And their test for
RxMaxSize register is fine.

> Is the silicium of both devices different enough to prevent the same
> exploit to happen ?

For this case, I don't think the device provide a invalid value
for the receive descriptors. However, the driver sees a different
value. That is why I say the memory is unbelievable.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1522219 — Re: [PATCH net 2/2] r8152: rx descriptor check

Hayes Wang <hayeswang@realtek.com> :
> Francois Romieu [mailto:romieu@fr.zoreil.com]
> > Sent: Friday, November 11, 2016 8:13 PM
> [...]
> > Invalid packet size corrupted receive descriptors in Realtek's device
> > reminds of CVE-2009-4537.
> 
> Do you mean that the driver would get a packet exceed the size
> which is set to RxMaxSize ?

If it was possible to get it wrong once, it should be possible to
get it wrong twice, especially if some part of the hardware design
is recycled. I don't mean anything else.

I won't speculate about some cache consistency issue or some badly
aborted dma transaction to explain the memory corruption.

-- 
Ueimor

[toc] | [prev] | [next] | [standalone]

#1524056 — RE: [PATCH net 2/2] r8152: rx descriptor check

Francois Romieu [mailto:romieu@fr.zoreil.com]
> Sent: Tuesday, November 15, 2016 9:11 AM
[...]
> If it was possible to get it wrong once, it should be possible to
> get it wrong twice, especially if some part of the hardware design
> is recycled. I don't mean anything else.

I agree with you. However, I have to let it could be reproduced
for confirming it.

Besides, the behavior is different for PCIe and USB device. There
is no action of DMA for USB device. It is done by the USB host
controller. And, the USB host controller wouldn't allow the device
sends a data which is more than the size of the buffer.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1520631 — Re: [PATCH net 2/2] r8152: rx descriptor check

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 11 Nov 2016 15:15:41 +0800

> For some platforms, the data in memory is not the same with the one
> from the device. That is, the data of memory is unbelievable. The
> check is used to find out this situation.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

I'm all for adding consistency checks, but I disagree with proceeding
in this manner for this.

If you add this patch now, there is a much smaller likelyhood that you
will work with a high priority to figure out _why_ this is happening.

For all we know this could be a platform bug in the DMA API for the
systems in question.

It could also be a bug elsewhere in the driver, either in setting up
the descriptor DMA mappings or how the chip is programmed.

Either way the true cause must be found before we start throwing
changes like this into the driver.

I'm not applying this series, sorry.

[toc] | [prev] | [next] | [standalone]

#1520660 — Re: [PATCH net 2/2] r8152: rx descriptor check

On 16-11-13 12:39 PM, David Miller wrote:
> From: Hayes Wang <hayeswang@realtek.com>
> Date: Fri, 11 Nov 2016 15:15:41 +0800
> 
>> For some platforms, the data in memory is not the same with the one
>> from the device. That is, the data of memory is unbelievable. The
>> check is used to find out this situation.
>>
>> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> 
> I'm all for adding consistency checks, but I disagree with proceeding
> in this manner for this.
> 
> If you add this patch now, there is a much smaller likelyhood that you
> will work with a high priority to figure out _why_ this is happening.
> 
> For all we know this could be a platform bug in the DMA API for the
> systems in question.
> 
> It could also be a bug elsewhere in the driver, either in setting up
> the descriptor DMA mappings or how the chip is programmed.
> 
> Either way the true cause must be found before we start throwing
> changes like this into the driver.

I agree.

The system I use it with is a 32-bit ppc476, with non-coherent RAM,
and using 16KB page sizes.

The dongle instantly becomes a lot more reliable when r8152.c is updated
to use usb_alloc_coherent() for URB buffers, rather than kmalloc().

Not sure why that would be though, as the USB stack normally would handle
kmalloc'd buffers just fine.  It is calling the appropriate routines,
which boil down to invalidating the dcache lines (for inbound bulk xfers)
as part of usb_submit_urb(), and yet the problem there persists.

It could be caused by cache-line sharing with other allocations, but that seems
unlikely as the kmalloc() size is 16384 bytes per buffer.  Perhaps the driver
is somehow accessing the buffer space again after doing usb_submit_urb()?
That would certainly produce this kind of behaviour.

Or maybe there's just a memory barrier missing somewhere in path.

The really weird thing is that ASIX-based dongles (which use a different driver)
don't have this problem, and yet they also use kmalloc'd buffers.

I have access to the test system only for a day or two a week,
and it takes a few hours to do a good test as to whether something helps or not.
I'll continue to poke at it as time and New Ideas permit.

New Ideas welcome!
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1520661 — Re: [PATCH net 2/2] r8152: rx descriptor check

On 16-11-13 03:34 PM, Mark Lord wrote:
>
> The system I use it with is a 32-bit ppc476, with non-coherent RAM,
> and using 16KB page sizes.
> 
> The dongle instantly becomes a lot more reliable when r8152.c is updated
> to use usb_alloc_coherent() for URB buffers, rather than kmalloc().
> 
> Not sure why that would be though, as the USB stack normally would handle
> kmalloc'd buffers just fine.  It is calling the appropriate routines,
> which boil down to invalidating the dcache lines (for inbound bulk xfers)
> as part of usb_submit_urb(), and yet the problem there persists.
> 
> It could be caused by cache-line sharing with other allocations, but that seems
> unlikely as the kmalloc() size is 16384 bytes per buffer.  Perhaps the driver
> is somehow accessing the buffer space again after doing usb_submit_urb()?
> That would certainly produce this kind of behaviour.
> 
> Or maybe there's just a memory barrier missing somewhere in path.
> 
> The really weird thing is that ASIX-based dongles (which use a different driver)
> don't have this problem, and yet they also use kmalloc'd buffers.
> 
> I have access to the test system only for a day or two a week,
> and it takes a few hours to do a good test as to whether something helps or not.
> I'll continue to poke at it as time and New Ideas permit.

Oh, and the problems did not exist with the 3.14.xx kernels and earlier.
They began to show up when we tried 3.16.xx and all newer kernels.

The difference there is that RX checksums were enabled in hardware as of 3.16.xx,
and thus the network stack began accepting bad packets from the r8152 driver.

I don't know if the ASIX driver uses hardware checksums or just software checksums.
That might explain why it is more reliable here.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1521370 — RE: [PATCH net 2/2] r8152: rx descriptor check

Mark Lord [mailto:mlord@pobox.com]
> Sent: Monday, November 14, 2016 4:34 AM
[...]
> Perhaps the driver
> is somehow accessing the buffer space again after doing usb_submit_urb()?
> That would certainly produce this kind of behaviour.

I don't think so. First, the driver only read the received buffer.
That is, the driver would not change (or write) the data. Second,
The driver would lose the point address of the received buffer
after submitting the urb to the USB host controller, until the
transfer is completed by the USB host controller. That is, the
driver doesn't how to access the buffer after calling usb_submit_urb().

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1521898 — Re: [PATCH net 2/2] r8152: rx descriptor check

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 14 Nov 2016 07:23:51 +0000

> Mark Lord [mailto:mlord@pobox.com]
>> Sent: Monday, November 14, 2016 4:34 AM
> [...]
>> Perhaps the driver
>> is somehow accessing the buffer space again after doing usb_submit_urb()?
>> That would certainly produce this kind of behaviour.
> 
> I don't think so. First, the driver only read the received buffer.
> That is, the driver would not change (or write) the data. Second,
> The driver would lose the point address of the received buffer
> after submitting the urb to the USB host controller, until the
> transfer is completed by the USB host controller. That is, the
> driver doesn't how to access the buffer after calling usb_submit_urb().

This is why it's most likely some DMA implementation issue or similar.

[toc] | [prev] | [next] | [standalone]

#1521359 — RE: [PATCH net 2/2] r8152: rx descriptor check

David Miller [mailto:davem@davemloft.net]
> Sent: Monday, November 14, 2016 1:40 AM
[...]
> If you add this patch now, there is a much smaller likelyhood that you
> will work with a high priority to figure out _why_ this is happening.
> 
> For all we know this could be a platform bug in the DMA API for the
> systems in question.
> 
> It could also be a bug elsewhere in the driver, either in setting up
> the descriptor DMA mappings or how the chip is programmed.
> 
> Either way the true cause must be found before we start throwing
> changes like this into the driver.

Our hw engineer could check our device, and I could check the
driver. However, for the other parts, such as the USB host
controller or memory, it is difficult for me to make sure whether
they are correct or not. I could only promise our devices and
driver work fine.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1519595 — [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

Fix the hw rx checksum is always enabled, and the user couldn't switch
it to sw rx checksum.

Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
the hw rx checksum for RTL_VER_02 is disabled after
commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 75c5168..0e42a78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1730,7 +1730,7 @@ static u8 r8152_rx_csum(struct r8152 *tp, struct rx_desc *rx_desc)
 	u8 checksum = CHECKSUM_NONE;
 	u32 opts2, opts3;
 
-	if (tp->version == RTL_VER_01 || tp->version == RTL_VER_02)
+	if (!(tp->netdev->features & NETIF_F_RXCSUM))
 		goto return_result;
 
 	opts2 = le32_to_cpu(rx_desc->opts2);
@@ -4307,6 +4307,11 @@ static int rtl8152_probe(struct usb_interface *intf,
 				NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
 				NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
 
+	if (tp->version == RTL_VER_01) {
+		netdev->features &= ~NETIF_F_RXCSUM;
+		netdev->hw_features &= ~NETIF_F_RXCSUM;
+	}
+
 	netdev->ethtool_ops = &ops;
 	netif_set_gso_max_size(netdev, RTL_LIMITED_TSO_SIZE);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1524067 — RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[...]
> Fix the hw rx checksum is always enabled, and the user couldn't switch
> it to sw rx checksum.
> 
> Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
> the hw rx checksum for RTL_VER_02 is disabled after
> commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.

Excuse me. If I want to re-send this one patch, should I let
RTL_VER_02 use rx hw checksum?

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1524447 — Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

On 16-11-17 09:14 AM, Mark Lord wrote:
..
> Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),

Note that the same behaviour also happens with the original kmalloc'd buffers.

> I can get it to fail extremely regularly by simply reducing the buffer size
> (agg_buf_sz) from 16KB down to 4KB.   This makes reproducing the issue
> much much easier -- the same problems do happen with the larger 16KB size,
> but much less often than with smaller sizes.

Increasing the buffer size to 64KB makes the problem much less frequent,
as one might expect.  Thus far I haven't seen it happen at all, but a longer
run (1-3 days) is needed to make sure.  This however is NOT a "fix".

> So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
> I see this behaviour every 10 (rx) URBs or so:
>
> First URB (number 593):
> [   34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
> [   34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
>
> Next URB (number 594):
> [   34.281172] r8152_check_rx_desc: rx_desc looks bad.
> [   34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
> [   34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768
>
> What the above sample shows, is the URB transfer buffer ran out of space in the middle
> of a packet, and the hardware then tried to just continue that same packet in the next URB,
> without an rx_desc header inserted.  The r8152.c driver always assumes the URB buffer begins
> with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..
>
> So until that driver bug is addressed, I would advise disabling hardware RX checksums
> for all chip versions, not only for version 02.
>
> It is not clear to me how the chip decides when to forward an rx URB to the host.
> If you could describe how that part works for us, then it would help in further
> understanding why fast systems (eg. a PC) don't generally notice the issue,
> while much slower embedded systems do see the issue regularly.

That last part is critical to understanding things:
How does the chip decide that a URB is "full enough" before sending it to the host?
Why does a really fast host see fewer packets jammed together into a single URB than a slower host?

The answers will help understand if there are more bugs to be found/fixed,
or if everything is explained by what has been observed thus far.

To recap:  the hardware sometimes fills a URB to the very end, and then continues the
current packet at the first byte of the following URB.  The r8152.c driver does NOT
handle this situation; instead it always interprets the first 24 bytes of every URB
as an "rx_desc" structure, without any kind of sanity/validation.  This results in
buffer overruns (it trusts the packet length field, even though the URB is too small
to hold such a packet), and other semi-random behaviour.

Using software rx checksums prevents Bad Things(tm) happening from most of this,
but even that is not perfect given the severity of the bug.

Cheers

[toc] | [prev] | [next] | [standalone]

#1524454 — Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

(resending.. not sure if the original had mailer errors)

On 16-11-16 10:36 PM, Hayes Wang wrote:
 > [...]
 >> Fix the hw rx checksum is always enabled, and the user couldn't switch
 >> it to sw rx checksum.
 >>
 >> Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
 >> the hw rx checksum for RTL_VER_02 is disabled after
 >> commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.
 >
 > Excuse me. If I want to re-send this one patch, should I let
 > RTL_VER_02 use rx hw checksum?

Definitely NOT.

I am still doing low-level tracing through the driver as time permits,
and just now found some really interesting evidence.

Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),
I can get it to fail extremely regularly by simply reducing the buffer size
(agg_buf_sz) from 16KB down to 4KB.   This makes reproducing the issue
much much easier -- the same problems do happen with the larger 16KB size,
but much less often than with smaller sizes.

So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
I see this behaviour every 10 (rx) URBs or so:

First URB (number 593):
[   34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
[   34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518

Next URB (number 594):
[   34.281172] r8152_check_rx_desc: rx_desc looks bad.
[   34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
[   34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768

What the above sample shows, is the URB transfer buffer ran out of space in the middle
of a packet, and the hardware then tried to just continue that same packet in the next URB,
without an rx_desc header inserted.  The r8152.c driver always assumes the URB buffer begins
with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..

So until that driver bug is addressed, I would advise disabling hardware RX checksums
for all chip versions, not only for version 02.

It is not clear to me how the chip decides when to forward an rx URB to the host.
If you could describe how that part works for us, then it would help in further
understanding why fast systems (eg. a PC) don't generally notice the issue,
while much slower embedded systems do see the issue regularly.

Thanks
Mark

[toc] | [prev] | [next] | [standalone]

#1525059 — RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

Mark Lord [mailto:mlord@pobox.com]
> Sent: Thursday, November 17, 2016 9:42 PM
[...]
> What the above sample shows, is the URB transfer buffer ran out of space in the
> middle
> of a packet, and the hardware then tried to just continue that same packet in the
> next URB,
> without an rx_desc header inserted.  The r8152.c driver always assumes the URB
> buffer begins
> with an rx_desc, so of course this behaviour produces really weird effects, and
> system crashes, etc..

The USB device wouldn't know the address and size of buffer. Only
the USB host controller knows. Therefore, the device sends the
data to host, and the host fills the memory. According to your
description, it seems the host splits the data from the device
into two different buffers (or URB transfers). I wonder if it would
occur. As far as I know, the host wouldn't allow the buffer size
less than the data length.

Our hw engineers need the log from the USB analyzer to confirm
what the device sends to the host. However, I don't think you
have USB analyzer to do this. I would try to reproduce the issue.
But, I am busy, so I don't think I would response quickly.

Besides, the maximum data length which the RTL8152 would send to
the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
wouldn't split it. However, you still see problems for it.

[...]
> It is not clear to me how the chip decides when to forward an rx URB to the host.
> If you could describe how that part works for us, then it would help in further
> understanding why fast systems (eg. a PC) don't generally notice the issue,
> while much slower embedded systems do see the issue regularly.

The driver expects the rx buffer would be

	rx_desc + a packet + padding to 8 alignment + 
	rx_desc + a packet + padding to 8 alignment + ...
	
Therefore, when a urb transfer is completed, the driver parsers
the buffer by this way. After the buffer is handled, it would
be submitted to the host, until the transfer is completed again.
If the submitting fail, the driver would try again later. The
urb->actual_length means how much data the host fills. The drive
uses it to check the end of the data. The urb->status mean if
the transfer is successful. The driver submits the urb to the
host directly if the status is not successful.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1525239 — Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

On 16-11-18 02:57 AM, Hayes Wang wrote:
..
> Besides, the maximum data length which the RTL8152 would send to
> the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
> wouldn't split it. However, you still see problems for it.

How does the RTL8152 know that the limit is 16KB,
rather than some other number?  Is this a hardwired number
in the hardware, or is it a parameter that the software
sends to the chip during initialization?

I have a USB analyzer, but it is difficult to figure out how
to program an appropriate trigger point for the capture,
since the problem (with 16KB URBs) takes minutes to hours
or even days to trigger.

And the output from the analyzer is in some proprietary format.
The in-kernel software analzer could be useful, but I have never
figured out how to use it.  :)

Since my earlier email, I have figured out another piece of the
puzzle with this dongle.

The first issue is that a packet sometimes begins in one URB,
and completes in the next URB, without an rx_desc at the start
of the second URB.  This I have already reported earlier.

But the driver, as written, sometimes accesses bytes outside
of the 16KB URB buffer, because it trusts the non-existent
rx_desc in these cases, and also because it accesses bytes
from the rx_desc without first checking whether there is
sufficient remaining space in the URB to hold an rx_desc.

These incorrect accesses sometimes touch memory outside
of the URB buffer.  Since the driver allocates all of its
rx URB buffers at once, they are highly likely to be
physically (and therefore virtually) adjacent in memory.

So mistakenly accessing beyond the end of one buffer will
often result in a read from memory of the next URB buffer.
Which causes a portion of it to be loaded in the the D-cache.

When that URB is subsequently filled by DMA, there then exists
a data-consistency issue:  the D-cache contains stale information
from before the latest DMA cycle.

So this explains the strange memory behaviour observed earlier on.
When I add a call to invalidate_dcache_range() to the driver
just before it begins examining a new rx URB, the problems go away.
So this confirms the observations.

Using non-cacheable RAM also makes the problem go away.
But neither is a fix for the real buffer overrun accesses in the driver.

Fix the "packet spans URBs" bug, and fix the driver to ALWAYS
test lengths/ranges before accessing the actual buffer,
and everything should begin working reliably.

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1527502 — Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

On 16-11-18 07:03 AM, Mark Lord wrote:
> On 16-11-18 02:57 AM, Hayes Wang wrote:
> ..
>> Besides, the maximum data length which the RTL8152 would send to
>> the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
>> wouldn't split it. However, you still see problems for it.
>
> How does the RTL8152 know that the limit is 16KB,
> rather than some other number?  Is this a hardwired number
> in the hardware, or is it a parameter that the software
> sends to the chip during initialization?
..
> The first issue is that a packet sometimes begins in one URB,
> and completes in the next URB, without an rx_desc at the start
> of the second URB.  This I have already reported earlier.

Long run tests over the weekend, with the invalidate_dcache_range() call
before the inner loop of r8152_rx_bottom(), turned up a few instances
where packets were truncated inside a 16384 byte URB buffer, without filling the URB.

[10.293228] r8152_rx_bottom: 4278 corrupted urb: head=9d210000 urb_offset=2856/3376 pkt_len(1518) exceeds remainder(496)
[10.304523] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
..
[   16.660431] r8152_rx_bottom: 7802 corrupted urb: head=9d1f8000 urb_offset=1544/2064 pkt_len(1518) exceeds remainder(496)
[   16.671719] r8152_dump_rx_desc: 044805ee 40480000 004005dc 46020006 00000000 00000000 rx_len=1518

The r8152.c driver attempted to build skb's for the entire packet size,
even though the 1518-byte packets had only 496-bytes of data in the URB.
It is not clear what the chip did with the rest of the packets in question,
but the next URBs in each case began with a new/real rx_desc and new packet.

There were also unconnected events during the test runs where the
test code noticed totally invalid rx_desc structs in the middles of URBs.
The stock driver would again have attempted to treat those as "valid" (ugh).

..
[   10.273906] r8152_check_rx_desc: rx_desc looks bad.
[   10.279012] r8152_rx_bottom: 4338 corrupted urb. head=9d210000 urb_offset=2856/3376 len_used=2880
[   10.288196] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 202f3a30 rx_len=12857

..
[    7.184565] r8152_check_rx_desc: rx_desc looks bad.
[    7.189657] r8152_rx_bottom: 1678 corrupted urb. head=9d210000 urb_offset=2856/3376 len_used=2880
[    7.198852] r8152_dump_rx_desc: a1388402 803c9001 84380810 a67c5c4c a77c782b c64c782b rx_len=1026
..
[   10.351251] r8152_check_rx_desc: rx_desc looks bad.
[   10.356356] r8152_rx_bottom: 4397 corrupted urb. head=9d20c000 urb_offset=4400/7984 len_used=4424
[   10.365543] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 202f3a30 rx_len=12857
..
[   10.518119] r8152_check_rx_desc: rx_desc looks bad.
[   10.523204] r8152_rx_bottom: 4458 corrupted urb. head=9d210000 urb_offset=4400/7984 len_used=4424
[   10.532416] r8152_dump_rx_desc: 54544120 6e3d5352 636f6c6f 65762c6b 343d7372 6464612c rx_len=16672
..

> But the driver, as written, sometimes accesses bytes outside
> of the 16KB URB buffer, because it trusts the non-existent
> rx_desc in these cases, and also because it accesses bytes
> from the rx_desc without first checking whether there is
> sufficient remaining space in the URB to hold an rx_desc.
>
> These incorrect accesses sometimes touch memory outside
> of the URB buffer.  Since the driver allocates all of its
> rx URB buffers at once, they are highly likely to be
> physically (and therefore virtually) adjacent in memory.
>
> So mistakenly accessing beyond the end of one buffer will
> often result in a read from memory of the next URB buffer.
> Which causes a portion of it to be loaded in the the D-cache.
>
> When that URB is subsequently filled by DMA, there then exists
> a data-consistency issue:  the D-cache contains stale information
> from before the latest DMA cycle.
>
> So this explains the strange memory behaviour observed earlier on.
> When I add a call to invalidate_dcache_range() to the driver
> just before it begins examining a new rx URB, the problems go away.
> So this confirms the observations.
>
> Using non-cacheable RAM also makes the problem go away.
> But neither is a fix for the real buffer overrun accesses in the driver.
>
> Fix the "packet spans URBs" bug, and fix the driver to ALWAYS
> test lengths/ranges before accessing the actual buffer,
> and everything should begin working reliably.

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web