Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 16:54 +0100, Emil Goode wrote: > Since the checks that need to be added in various places are all in > the same subsystem I think it could be done in as little as one patch? Yes, please definitely. Best for bisectability. > If nobody have any objections I will try removing the generic check and > introduce checks where nessecary. Thank you Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 16:54 +0100, Emil Goode wrote: Since the checks that need to be added in various places are all in the same subsystem I think it could be done in as little as one patch? Yes, please definitely. Best for bisectability. If nobody have any objections I will try removing the generic check and introduce checks where nessecary. Thank you Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, Feb 10, 2014 at 02:05:20PM +0100, Bjørn Mork wrote: > Oliver Neukum writes: > > On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: > >> On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: > > > >> > Well, then how about simply removing the check? > >> > It seems to have outlived its usefulness. > >> > > >> > Regards > >> > Oliver > >> > > >> > > >> > >> I did consider that and I think it is probably the best thing to do. > >> However, I think the removal of the check could have negative effects > >> on the other minidrivers, at least the qmi_wwan minidriver explicitly > >> states that it is depending on this check to be made in rx_complete(). > > > > . > > No need to do that. I had the exact same reaction myself :-) > > Anyway, I put that comment there mostly as a note to myself why the > check would be redundant at that point. I don't see any problem with > removing the generic check in usbnet as long as we add similar checks > whereever they are needed, like in qmi_wwan. I think it could be worth the trouble of removing the generic check in the usbnet module. I believe that if you define your own rx_fixup callback then the usbnet module should not make it's own assumptions on what packets to discard. Since the checks that need to be added in various places are all in the same subsystem I think it could be done in as little as one patch? > Note that usbnet_skb_return will be one of those places. It calls > eth_type_trans() on the skb, so it should verify that the length is at > least ETH_HLEN first. > > > Oh well. But how about merging it with FLAG_MULTI_PACKET? > > I really don't want to add more flags. There is a point where enough > > flags make absurd having a common code. We are closing in on that point. > > Agreed. I would even say we are past that point... If nobody have any objections I will try removing the generic check and introduce checks where nessecary. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
Oliver Neukum writes: > On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: >> On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: > >> > Well, then how about simply removing the check? >> > It seems to have outlived its usefulness. >> > >> >Regards >> >Oliver >> > >> > >> >> I did consider that and I think it is probably the best thing to do. >> However, I think the removal of the check could have negative effects >> on the other minidrivers, at least the qmi_wwan minidriver explicitly >> states that it is depending on this check to be made in rx_complete(). > > . No need to do that. I had the exact same reaction myself :-) Anyway, I put that comment there mostly as a note to myself why the check would be redundant at that point. I don't see any problem with removing the generic check in usbnet as long as we add similar checks whereever they are needed, like in qmi_wwan. Note that usbnet_skb_return will be one of those places. It calls eth_type_trans() on the skb, so it should verify that the length is at least ETH_HLEN first. > Oh well. But how about merging it with FLAG_MULTI_PACKET? > I really don't want to add more flags. There is a point where enough > flags make absurd having a common code. We are closing in on that point. Agreed. I would even say we are past that point... Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2 v2] usbnet: fix bad header length bug
From: Oliver Neukum > . Oh well. But how about merging it with FLAG_MULTI_PACKET? > I really don't want to add more flags. There is a point where enough > flags make absurd having a common code. We are closing in on that point. Any sub-driver that supports multi-packet either has to use stupidly long URB and/or set the rx_urb_size to a multiple of the usb transfer size. It will also have to detect illegal short headers. Actually it might be worth double-checking the encapsulations used. IIRC the ax88179_178a uses different headers for tx and rx. So there might be some that support multi-packet but still have a short(ish) limit on the bulk receive size (before the short fragment). I'm sat at the wrong desk to look at the code... David
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: > On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: > > On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: > > > The AX88772B occasionally send rx packets that cross urb boundaries > > > and the remaining partial packet is sent with no hardware header. > > > When the buffer with a partial packet is of less number of octets > > > than the value of hard_header_len the buffer is discarded by the > > > usbnet module. This is causing dropped packages and error messages > > > in dmesg. > > > > > > This can be reproduced by using ping with a packet size > > > between 1965-1976. > > > > Well, then how about simply removing the check? > > It seems to have outlived its usefulness. > > > > Regards > > Oliver > > > > > > I did consider that and I think it is probably the best thing to do. > However, I think the removal of the check could have negative effects > on the other minidrivers, at least the qmi_wwan minidriver explicitly > states that it is depending on this check to be made in rx_complete(). . Oh well. But how about merging it with FLAG_MULTI_PACKET? I really don't want to add more flags. There is a point where enough flags make absurd having a common code. We are closing in on that point. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: > On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: > > The AX88772B occasionally send rx packets that cross urb boundaries > > and the remaining partial packet is sent with no hardware header. > > When the buffer with a partial packet is of less number of octets > > than the value of hard_header_len the buffer is discarded by the > > usbnet module. This is causing dropped packages and error messages > > in dmesg. > > > > This can be reproduced by using ping with a packet size > > between 1965-1976. > > Well, then how about simply removing the check? > It seems to have outlived its usefulness. > > Regards > Oliver > > I did consider that and I think it is probably the best thing to do. However, I think the removal of the check could have negative effects on the other minidrivers, at least the qmi_wwan minidriver explicitly states that it is depending on this check to be made in rx_complete(). For safety the check could be added at the top of the rx_fixup callback of the affected minidrivers. There are devices that depend on the usbnet module that do not have a rx_fixup callback assigned to it's driver_info struct, the check could be added for these in the rx_process function. This led me to think it would be a lot of noise about a small check :) My conclusion is that 12 rx_fixup callbacks might need the check to be added. There are 18 driver_info structs with no rx_fixup callback assigned, these devices might need the check to be added to the rx_process function. Patches could be sent out to notify the affected minidrivers of the change and hopefully someone has the hardware to test if it's necessary to add the check to the minidriver? I'm happy to do this if it seem like a good idea. Best regards, Emil Goode -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: The AX88772B occasionally send rx packets that cross urb boundaries and the remaining partial packet is sent with no hardware header. When the buffer with a partial packet is of less number of octets than the value of hard_header_len the buffer is discarded by the usbnet module. This is causing dropped packages and error messages in dmesg. This can be reproduced by using ping with a packet size between 1965-1976. Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver I did consider that and I think it is probably the best thing to do. However, I think the removal of the check could have negative effects on the other minidrivers, at least the qmi_wwan minidriver explicitly states that it is depending on this check to be made in rx_complete(). For safety the check could be added at the top of the rx_fixup callback of the affected minidrivers. There are devices that depend on the usbnet module that do not have a rx_fixup callback assigned to it's driver_info struct, the check could be added for these in the rx_process function. This led me to think it would be a lot of noise about a small check :) My conclusion is that 12 rx_fixup callbacks might need the check to be added. There are 18 driver_info structs with no rx_fixup callback assigned, these devices might need the check to be added to the rx_process function. Patches could be sent out to notify the affected minidrivers of the change and hopefully someone has the hardware to test if it's necessary to add the check to the minidriver? I'm happy to do this if it seem like a good idea. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: The AX88772B occasionally send rx packets that cross urb boundaries and the remaining partial packet is sent with no hardware header. When the buffer with a partial packet is of less number of octets than the value of hard_header_len the buffer is discarded by the usbnet module. This is causing dropped packages and error messages in dmesg. This can be reproduced by using ping with a packet size between 1965-1976. Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver I did consider that and I think it is probably the best thing to do. However, I think the removal of the check could have negative effects on the other minidrivers, at least the qmi_wwan minidriver explicitly states that it is depending on this check to be made in rx_complete(). censored. Oh well. But how about merging it with FLAG_MULTI_PACKET? I really don't want to add more flags. There is a point where enough flags make absurd having a common code. We are closing in on that point. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2 v2] usbnet: fix bad header length bug
From: Oliver Neukum censored. Oh well. But how about merging it with FLAG_MULTI_PACKET? I really don't want to add more flags. There is a point where enough flags make absurd having a common code. We are closing in on that point. Any sub-driver that supports multi-packet either has to use stupidly long URB and/or set the rx_urb_size to a multiple of the usb transfer size. It will also have to detect illegal short headers. Actually it might be worth double-checking the encapsulations used. IIRC the ax88179_178a uses different headers for tx and rx. So there might be some that support multi-packet but still have a short(ish) limit on the bulk receive size (before the short fragment). I'm sat at the wrong desk to look at the code... David
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
Oliver Neukum oli...@neukum.org writes: On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver I did consider that and I think it is probably the best thing to do. However, I think the removal of the check could have negative effects on the other minidrivers, at least the qmi_wwan minidriver explicitly states that it is depending on this check to be made in rx_complete(). censored. No need to do that. I had the exact same reaction myself :-) Anyway, I put that comment there mostly as a note to myself why the check would be redundant at that point. I don't see any problem with removing the generic check in usbnet as long as we add similar checks whereever they are needed, like in qmi_wwan. Note that usbnet_skb_return will be one of those places. It calls eth_type_trans() on the skb, so it should verify that the length is at least ETH_HLEN first. Oh well. But how about merging it with FLAG_MULTI_PACKET? I really don't want to add more flags. There is a point where enough flags make absurd having a common code. We are closing in on that point. Agreed. I would even say we are past that point... Bjørn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, Feb 10, 2014 at 02:05:20PM +0100, Bjørn Mork wrote: Oliver Neukum oli...@neukum.org writes: On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote: On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote: Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver I did consider that and I think it is probably the best thing to do. However, I think the removal of the check could have negative effects on the other minidrivers, at least the qmi_wwan minidriver explicitly states that it is depending on this check to be made in rx_complete(). censored. No need to do that. I had the exact same reaction myself :-) Anyway, I put that comment there mostly as a note to myself why the check would be redundant at that point. I don't see any problem with removing the generic check in usbnet as long as we add similar checks whereever they are needed, like in qmi_wwan. I think it could be worth the trouble of removing the generic check in the usbnet module. I believe that if you define your own rx_fixup callback then the usbnet module should not make it's own assumptions on what packets to discard. Since the checks that need to be added in various places are all in the same subsystem I think it could be done in as little as one patch? Note that usbnet_skb_return will be one of those places. It calls eth_type_trans() on the skb, so it should verify that the length is at least ETH_HLEN first. Oh well. But how about merging it with FLAG_MULTI_PACKET? I really don't want to add more flags. There is a point where enough flags make absurd having a common code. We are closing in on that point. Agreed. I would even say we are past that point... If nobody have any objections I will try removing the generic check and introduce checks where nessecary. Best regards, Emil Goode -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: > The AX88772B occasionally send rx packets that cross urb boundaries > and the remaining partial packet is sent with no hardware header. > When the buffer with a partial packet is of less number of octets > than the value of hard_header_len the buffer is discarded by the > usbnet module. This is causing dropped packages and error messages > in dmesg. > > This can be reproduced by using ping with a packet size > between 1965-1976. Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v2] usbnet: fix bad header length bug
On Mon, 2014-02-10 at 00:06 +0100, Emil Goode wrote: The AX88772B occasionally send rx packets that cross urb boundaries and the remaining partial packet is sent with no hardware header. When the buffer with a partial packet is of less number of octets than the value of hard_header_len the buffer is discarded by the usbnet module. This is causing dropped packages and error messages in dmesg. This can be reproduced by using ping with a packet size between 1965-1976. Well, then how about simply removing the check? It seems to have outlived its usefulness. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
