Re: [PATCH] packet: Allow packets with only a header (but no payload)
On 2015-07-31 00:15, Martin Blumenstingl wrote: > On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn wrote: >> Martin, to return to your initial statement that PPPoE PADI packets can >> have a zero payload: the PPPoE RFC states that PADI packets "MUST >> contain exactly one TAG of TAG_TYPE Service-Name, indicating the >> service the Host is requesting, and any number of other TAG types." >> (RFC 2516, 5.1). Is the observed behavior (no payload) perhaps >> incorrect? > As far as I can see you are right, but the real world seems to be different. > My ISP for example lists the PPPoE connection settings, but they are > nowhere mentioning the "service name". > > I have also re-read pppd's source code again and that seems to confirm > what you are reading in the RFC: Leaving the service name away makes > seems to violate the RFC, but pppd still accepts those configurations. > >> Even if it is, if this is breaking established userspace expectations, >> we should look into it. Ethernet specifies a minimum payload size of >> 46 on the wire, but perhaps that is handled with padding, so that >> 0 length should be valid within the stack. Also, there may be other >> valid uses of 0 length payload on top of link layers that are not Ethernet. > Good catch. I would also like to note that the documentation for > "hard_header_len" describes it as "Hardware header length". When the > purpose of this field we should check whether the documentation should > be updated to "Minimum hardware header length" -> that would mean the > condition has to be a "len < hard_header_len" instead of a "len <= > hard_header_len" (as it is now). > > PS: I have also added the pppd maintainer (Paul Mackerras) to this > thread because I think he should know about this issue (and he can > probably provide more details if required). > As a quick summary for him: linux >= 3.19 rejects PADI packets when > no service name is configured. Any news on this? Users are complaining about this regression: https://dev.openwrt.org/ticket/20707 - Felix -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On Sat, Nov 7, 2015 at 8:11 AM, Felix Fietkau wrote:
> On 2015-07-31 00:15, Martin Blumenstingl wrote:
>> On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn wrote:
>>> Martin, to return to your initial statement that PPPoE PADI packets can
>>> have a zero payload: the PPPoE RFC states that PADI packets "MUST
>>> contain exactly one TAG of TAG_TYPE Service-Name, indicating the
>>> service the Host is requesting, and any number of other TAG types."
>>> (RFC 2516, 5.1). Is the observed behavior (no payload) perhaps
>>> incorrect?
>> As far as I can see you are right, but the real world seems to be different.
>> My ISP for example lists the PPPoE connection settings, but they are
>> nowhere mentioning the "service name".
>>
>> I have also re-read pppd's source code again and that seems to confirm
>> what you are reading in the RFC: Leaving the service name away makes
>> seems to violate the RFC, but pppd still accepts those configurations.
>>
>>> Even if it is, if this is breaking established userspace expectations,
>>> we should look into it. Ethernet specifies a minimum payload size of
>>> 46 on the wire, but perhaps that is handled with padding, so that
>>> 0 length should be valid within the stack. Also, there may be other
>>> valid uses of 0 length payload on top of link layers that are not Ethernet.
>> Good catch. I would also like to note that the documentation for
>> "hard_header_len" describes it as "Hardware header length". When the
>> purpose of this field we should check whether the documentation should
>> be updated to "Minimum hardware header length" -> that would mean the
>> condition has to be a "len < hard_header_len" instead of a "len <=
>> hard_header_len" (as it is now).
>>
>> PS: I have also added the pppd maintainer (Paul Mackerras) to this
>> thread because I think he should know about this issue (and he can
>> probably provide more details if required).
>> As a quick summary for him: linux >= 3.19 rejects PADI packets when
>> no service name is configured.
> Any news on this? Users are complaining about this regression:
> https://dev.openwrt.org/ticket/20707
I took another look. This hinges on the question what the contract with
device drivers is on skb network data and length. Is passing an skb with
skb->len == 0 to ndo_start_xmit allowed?
>From what I gather from the ethernet spec [1], sending frames with an
empty head is allowed on that medium, at least.
A quick scan of a few drivers and the loopback path also does not show
anything that would break. In some cases, skb_network_header points
beyond the end of the buffer (ETH_HLEN), but the length is correctly
reported as 0.
The tap device can also generate packets consisting of only a link layer
header: compares len < ETH_HLEN in tun_get_user.
So, I think that this change should be correct:
static bool ll_header_truncated(const struct net_device *dev, int len)
{
- /* net device doesn't like empty head */
- if (unlikely(len <= dev->hard_header_len)) {
+ if (unlikely(len < dev->hard_header_len)) {
but a definitive answer would require an audit of all device drivers
(including bonding, ..) or at least the certainty that it has always
been correct to send a packet of only link layer header to
ndo_start_xmit.
[1] IEEE 802.3™-2012 – Section One, {3.2.8, 4.2.3.3}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On 2015-11-09 18:53, Willem de Bruijn wrote:
> On Sat, Nov 7, 2015 at 8:11 AM, Felix Fietkau wrote:
>> On 2015-07-31 00:15, Martin Blumenstingl wrote:
>>> On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn
>>> wrote:
Martin, to return to your initial statement that PPPoE PADI packets can
have a zero payload: the PPPoE RFC states that PADI packets "MUST
contain exactly one TAG of TAG_TYPE Service-Name, indicating the
service the Host is requesting, and any number of other TAG types."
(RFC 2516, 5.1). Is the observed behavior (no payload) perhaps
incorrect?
>>> As far as I can see you are right, but the real world seems to be different.
>>> My ISP for example lists the PPPoE connection settings, but they are
>>> nowhere mentioning the "service name".
>>>
>>> I have also re-read pppd's source code again and that seems to confirm
>>> what you are reading in the RFC: Leaving the service name away makes
>>> seems to violate the RFC, but pppd still accepts those configurations.
>>>
Even if it is, if this is breaking established userspace expectations,
we should look into it. Ethernet specifies a minimum payload size of
46 on the wire, but perhaps that is handled with padding, so that
0 length should be valid within the stack. Also, there may be other
valid uses of 0 length payload on top of link layers that are not Ethernet.
>>> Good catch. I would also like to note that the documentation for
>>> "hard_header_len" describes it as "Hardware header length". When the
>>> purpose of this field we should check whether the documentation should
>>> be updated to "Minimum hardware header length" -> that would mean the
>>> condition has to be a "len < hard_header_len" instead of a "len <=
>>> hard_header_len" (as it is now).
>>>
>>> PS: I have also added the pppd maintainer (Paul Mackerras) to this
>>> thread because I think he should know about this issue (and he can
>>> probably provide more details if required).
>>> As a quick summary for him: linux >= 3.19 rejects PADI packets when
>>> no service name is configured.
>> Any news on this? Users are complaining about this regression:
>> https://dev.openwrt.org/ticket/20707
>
> I took another look. This hinges on the question what the contract with
> device drivers is on skb network data and length. Is passing an skb with
> skb->len == 0 to ndo_start_xmit allowed?
>
> From what I gather from the ethernet spec [1], sending frames with an
> empty head is allowed on that medium, at least.
>
> A quick scan of a few drivers and the loopback path also does not show
> anything that would break. In some cases, skb_network_header points
> beyond the end of the buffer (ETH_HLEN), but the length is correctly
> reported as 0.
>
> The tap device can also generate packets consisting of only a link layer
> header: compares len < ETH_HLEN in tun_get_user.
>
> So, I think that this change should be correct:
>
> static bool ll_header_truncated(const struct net_device *dev, int len)
> {
> - /* net device doesn't like empty head */
> - if (unlikely(len <= dev->hard_header_len)) {
> + if (unlikely(len < dev->hard_header_len)) {
>
> but a definitive answer would require an audit of all device drivers
> (including bonding, ..) or at least the certainty that it has always
> been correct to send a packet of only link layer header to
> ndo_start_xmit.
>
> [1] IEEE 802.3™-2012 – Section One, {3.2.8, 4.2.3.3}
Yeah, I agree that such an audit is required. However, I think it's
*much* more important to add this change as soon as possible to fix the
regression. The old code may have had theoretical driver issues, but the
current code breaks real-world user setups.
- Felix
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
Hello. On 11/21/2015 3:50 AM, Martin Blumenstingl wrote: 9c70776 added validation for the packet size in packet_snd. This change Please run your patch thru scripts/checkpatch.pl -- it now enforces certain commit citing style. enforced that every packet needs a header with at least hard_header_len bytes and at least one byte payload. This fixes PPPoE connections which do not have a "Service" or "Host-Uniq" configured (which is violating the spec, but is still widely used in real-world setups). Those are currently failing with the following message: "pppd: packet size is too short (24 <= 24)" Signed-off-by: Martin Blumenstingl [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On Tue, Jul 21, 2015 at 12:14 PM, Martin Blumenstingl wrote: > 9c70776 added validation for the packet size in packet_snd. This change > enforced that every packet needs a long enough header and at least one > byte payload. > > However, when trying to establish a PPPoE connection the following message > is printed every time a PPPoE discovery packet is sent: > pppd: packet size is too short (24 <= 24) > > From what I can see in the PPPoE code the "PADI" discovery packet can > consist of only a header with no payload (when there is neither a service > name nor a Host-Uniq configured). Interesting. 9c7077622dd9 only extended the check from tpacket_snd to packet_snd to make the two paths equivalent. The existing check had the ominous statement /* net device doesn't like empty head */ so allowing a header-only packet while correct in your case may not be safe in some edge cases (specific device drivers?). This was also discussed previously http://www.spinics.net/lists/netdev/msg309677.html In any case, I don't think that reverting the patch and restoring the old inconsistent state is a fix. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
Hi Willem, On Tue, Jul 21, 2015 at 6:28 PM, Willem de Bruijn wrote: > Interesting. 9c7077622dd9 only extended the check from tpacket_snd to > packet_snd to make the two paths equivalent. The existing check had the > ominous statement > > /* net device doesn't like empty head */ OK, I guess it's best to find out what the purpose of this comment is. > so allowing a header-only packet while correct in your case may not be > safe in some edge cases (specific device drivers?). I'm wondering how a good fix would look like (I can think of a few things, like renaming hard_header_len to something min_packet_size)? I am open for suggestions since I have zero knowledge about the inner workings of the packet framework. > This was also discussed previously > > http://www.spinics.net/lists/netdev/msg309677.html > > In any case, I don't think that reverting the patch and restoring the old > inconsistent state is a fix. I totally agree with you that it's a bad fix if this means that we could break other drivers. My primary goal was to fix PPPoE connections - I guess I should have simply added "RFC" to the subject. Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On Tue, Jul 21, 2015 at 12:38 PM, Martin Blumenstingl wrote: > Hi Willem, > > On Tue, Jul 21, 2015 at 6:28 PM, Willem de Bruijn wrote: >> Interesting. 9c7077622dd9 only extended the check from tpacket_snd to >> packet_snd to make the two paths equivalent. The existing check had the >> ominous statement >> >> /* net device doesn't like empty head */ > OK, I guess it's best to find out what the purpose of this comment is. > >> so allowing a header-only packet while correct in your case may not be >> safe in some edge cases (specific device drivers?). > I'm wondering how a good fix would look like (I can think of a few > things, like renaming hard_header_len to something min_packet_size)? > I am open for suggestions since I have zero knowledge about the inner > workings of the packet framework. I don't see a simple way of verifying the safety of allowing packets without data short of a code audit, which would be huge, especially when taking device driver logic into account. Perhaps someone remembers why that statement was added and what edge case(s) it refers to. I'm afraid that I don't. It was added in 69e3c75f4d54. I added the author to this thread. >> This was also discussed previously >> >> http://www.spinics.net/lists/netdev/msg309677.html >> >> In any case, I don't think that reverting the patch and restoring the old >> inconsistent state is a fix. > I totally agree with you that it's a bad fix if this means that we > could break other drivers. > My primary goal was to fix PPPoE connections - I guess I should have > simply added "RFC" to the subject. > > > Regards, > Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
Hi Johann, On Tue, Jul 21, 2015 at 6:51 PM, Willem de Bruijn wrote: > I don't see a simple way of verifying the safety of allowing packets > without data short of a code audit, which would be huge, especially > when taking device driver logic into account. Perhaps someone > remembers why that statement was added and what edge case(s) > it refers to. I'm afraid that I don't. It was added in 69e3c75f4d54. I > added the author to this thread. I know it's summer (and thus vacation-time), but did you already have a chance to look into this? Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On Mon, Jul 27, 2015 at 6:35 PM, Martin Blumenstingl wrote: > Hi Johann, > > On Tue, Jul 21, 2015 at 6:51 PM, Willem de Bruijn wrote: >> I don't see a simple way of verifying the safety of allowing packets >> without data short of a code audit, which would be huge, especially >> when taking device driver logic into account. Perhaps someone >> remembers why that statement was added and what edge case(s) >> it refers to. I'm afraid that I don't. It was added in 69e3c75f4d54. I >> added the author to this thread. > I know it's summer (and thus vacation-time), but did you already have > a chance to look into this? Martin, to return to your initial statement that PPPoE PADI packets can have a zero payload: the PPPoE RFC states that PADI packets "MUST contain exactly one TAG of TAG_TYPE Service-Name, indicating the service the Host is requesting, and any number of other TAG types." (RFC 2516, 5.1). Is the observed behavior (no payload) perhaps incorrect? Even if it is, if this is breaking established userspace expectations, we should look into it. Ethernet specifies a minimum payload size of 46 on the wire, but perhaps that is handled with padding, so that 0 length should be valid within the stack. Also, there may be other valid uses of 0 length payload on top of link layers that are not Ethernet. > > Regards, > Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] packet: Allow packets with only a header (but no payload)
On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn wrote: > Martin, to return to your initial statement that PPPoE PADI packets can > have a zero payload: the PPPoE RFC states that PADI packets "MUST > contain exactly one TAG of TAG_TYPE Service-Name, indicating the > service the Host is requesting, and any number of other TAG types." > (RFC 2516, 5.1). Is the observed behavior (no payload) perhaps > incorrect? As far as I can see you are right, but the real world seems to be different. My ISP for example lists the PPPoE connection settings, but they are nowhere mentioning the "service name". I have also re-read pppd's source code again and that seems to confirm what you are reading in the RFC: Leaving the service name away makes seems to violate the RFC, but pppd still accepts those configurations. > Even if it is, if this is breaking established userspace expectations, > we should look into it. Ethernet specifies a minimum payload size of > 46 on the wire, but perhaps that is handled with padding, so that > 0 length should be valid within the stack. Also, there may be other > valid uses of 0 length payload on top of link layers that are not Ethernet. Good catch. I would also like to note that the documentation for "hard_header_len" describes it as "Hardware header length". When the purpose of this field we should check whether the documentation should be updated to "Minimum hardware header length" -> that would mean the condition has to be a "len < hard_header_len" instead of a "len <= hard_header_len" (as it is now). PS: I have also added the pppd maintainer (Paul Mackerras) to this thread because I think he should know about this issue (and he can probably provide more details if required). As a quick summary for him: linux >= 3.19 rejects PADI packets when no service name is configured. Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
