netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] packet: Allow packets with only a header (but no payload)
@ 2015-07-21 16:14 Martin Blumenstingl
  2015-07-21 16:28 ` Willem de Bruijn
  2015-11-21  0:50 ` Martin Blumenstingl
  0 siblings, 2 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2015-07-21 16:14 UTC (permalink / raw)
  To: netdev; +Cc: willemb, edumazet, dborkman, davem, mostrows, Martin Blumenstingl

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).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 net/packet/af_packet.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c9e8741..d983f8f 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2199,18 +2199,6 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 	sock_wfree(skb);
 }
 
-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)) {
-		net_warn_ratelimited("%s: packet size is too short (%d <= %d)\n",
-				     current->comm, len, dev->hard_header_len);
-		return true;
-	}
-
-	return false;
-}
-
 static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		void *frame, struct net_device *dev, int size_max,
 		__be16 proto, unsigned char *addr, int hlen)
@@ -2286,8 +2274,14 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err < 0))
 			return -EINVAL;
 	} else if (dev->hard_header_len) {
-		if (ll_header_truncated(dev, tp_len))
+		/* net device doesn't like empty head */
+		if (unlikely(len <= dev->hard_header_len)) {
+			net_warn_ratelimited("%s: packet size is too short "
+					"(%d <= %d)\n",
+					current->comm, len,
+					dev->hard_header_len);
 			return -EINVAL;
+		}
 
 		skb_push(skb, dev->hard_header_len);
 		err = skb_store_bits(skb, 0, data,
@@ -2624,8 +2618,13 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		if (unlikely(offset < 0))
 			goto out_free;
 	} else {
-		if (ll_header_truncated(dev, len))
+		if (unlikely(len < dev->hard_header_len)) {
+			net_warn_ratelimited("%s: packet size is shorter than "
+					"minimum header size (%d < %d)\n",
+					current->comm, len,
+					dev->hard_header_len);
 			goto out_free;
+		}
 	}
 
 	/* Returns -EFAULT on error */
-- 
2.4.6

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-21 16:14 [PATCH] packet: Allow packets with only a header (but no payload) Martin Blumenstingl
@ 2015-07-21 16:28 ` Willem de Bruijn
  2015-07-21 16:38   ` Martin Blumenstingl
  2015-11-21  0:50 ` Martin Blumenstingl
  1 sibling, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2015-07-21 16:28 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Network Development, Eric Dumazet, Daniel Borkmann, David Miller,
	mostrows

On Tue, Jul 21, 2015 at 12:14 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-21 16:28 ` Willem de Bruijn
@ 2015-07-21 16:38   ` Martin Blumenstingl
  2015-07-21 16:51     ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2015-07-21 16:38 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Network Development, Eric Dumazet, Daniel Borkmann, David Miller,
	mostrows

Hi Willem,

On Tue, Jul 21, 2015 at 6:28 PM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-21 16:38   ` Martin Blumenstingl
@ 2015-07-21 16:51     ` Willem de Bruijn
  2015-07-27 22:35       ` Martin Blumenstingl
  0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2015-07-21 16:51 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Network Development, Eric Dumazet, Daniel Borkmann, David Miller,
	johann.baudy

On Tue, Jul 21, 2015 at 12:38 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Willem,
>
> On Tue, Jul 21, 2015 at 6:28 PM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-21 16:51     ` Willem de Bruijn
@ 2015-07-27 22:35       ` Martin Blumenstingl
  2015-07-29  6:05         ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2015-07-27 22:35 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Network Development, Eric Dumazet, Daniel Borkmann, David Miller,
	johann.baudy

Hi Johann,

On Tue, Jul 21, 2015 at 6:51 PM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-27 22:35       ` Martin Blumenstingl
@ 2015-07-29  6:05         ` Willem de Bruijn
  2015-07-30 22:15           ` Martin Blumenstingl
  0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2015-07-29  6:05 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Network Development, Eric Dumazet, Daniel Borkmann, David Miller,
	johann.baudy

On Mon, Jul 27, 2015 at 6:35 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Johann,
>
> On Tue, Jul 21, 2015 at 6:51 PM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-29  6:05         ` Willem de Bruijn
@ 2015-07-30 22:15           ` Martin Blumenstingl
  2015-11-07 13:11             ` Felix Fietkau
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2015-07-30 22:15 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Network Development, Eric Dumazet, David Miller, johann.baudy, paulus

On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-30 22:15           ` Martin Blumenstingl
@ 2015-11-07 13:11             ` Felix Fietkau
  2015-11-09 17:53               ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Felix Fietkau @ 2015-11-07 13:11 UTC (permalink / raw)
  To: Martin Blumenstingl, Willem de Bruijn
  Cc: Network Development, Eric Dumazet, David Miller, johann.baudy, paulus

On 2015-07-31 00:15, Martin Blumenstingl wrote:
> On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-11-07 13:11             ` Felix Fietkau
@ 2015-11-09 17:53               ` Willem de Bruijn
  2015-11-09 19:02                 ` Felix Fietkau
  0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2015-11-09 17:53 UTC (permalink / raw)
  To: Felix Fietkau
  Cc: Martin Blumenstingl, Network Development, Eric Dumazet,
	David Miller, johann.baudy, paulus, Daniel Borkmann

On Sat, Nov 7, 2015 at 8:11 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2015-07-31 00:15, Martin Blumenstingl wrote:
>> On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn <willemb@google.com> 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}

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-11-09 17:53               ` Willem de Bruijn
@ 2015-11-09 19:02                 ` Felix Fietkau
  0 siblings, 0 replies; 14+ messages in thread
From: Felix Fietkau @ 2015-11-09 19:02 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Martin Blumenstingl, Network Development, Eric Dumazet,
	David Miller, johann.baudy, paulus, Daniel Borkmann

On 2015-11-09 18:53, Willem de Bruijn wrote:
> On Sat, Nov 7, 2015 at 8:11 AM, Felix Fietkau <nbd@openwrt.org> wrote:
>> On 2015-07-31 00:15, Martin Blumenstingl wrote:
>>> On Wed, Jul 29, 2015 at 8:05 AM, Willem de Bruijn <willemb@google.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] packet: Allow packets with only a header (but no payload)
  2015-07-21 16:14 [PATCH] packet: Allow packets with only a header (but no payload) Martin Blumenstingl
  2015-07-21 16:28 ` Willem de Bruijn
@ 2015-11-21  0:50 ` Martin Blumenstingl
  2015-11-21 21:32   ` Sergei Shtylyov
  2015-11-22 16:46   ` [PATCH v3] " Martin Blumenstingl
  1 sibling, 2 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2015-11-21  0:50 UTC (permalink / raw)
  To: netdev
  Cc: willemb, edumazet, dborkman, davem, mostrows, nbd, Martin Blumenstingl

9c70776 added validation for the packet size in packet_snd. This change
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 <martin.blumenstingl@googlemail.com>
---
v2: Simply change the existing logic in ll_header_truncated instead of
    splitting it and having multiple checks.

 include/linux/netdevice.h | 3 ++-
 net/packet/af_packet.c    | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 67bfac1..1f42cb7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1398,7 +1398,8 @@ enum netdev_priv_flags {
  *	@dma:		DMA channel
  *	@mtu:		Interface MTU value
  *	@type:		Interface hardware type
- *	@hard_header_len: Hardware header length
+ *	@hard_header_len: Hardware header length, which means that this is the
+ * 				minimum size of a packet.
  *
  *	@needed_headroom: Extra headroom the hardware may need, but not in all
  *			  cases can this be guaranteed
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1cf928f..992396a 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2329,8 +2329,8 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 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)) {
-		net_warn_ratelimited("%s: packet size is too short (%d <= %d)\n",
+	if (unlikely(len < dev->hard_header_len)) {
+		net_warn_ratelimited("%s: packet size is too short (%d < %d)\n",
 				     current->comm, len, dev->hard_header_len);
 		return true;
 	}
-- 
2.6.2

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] packet: Allow packets with only a header (but no payload)
  2015-11-21  0:50 ` Martin Blumenstingl
@ 2015-11-21 21:32   ` Sergei Shtylyov
  2015-11-22 16:46   ` [PATCH v3] " Martin Blumenstingl
  1 sibling, 0 replies; 14+ messages in thread
From: Sergei Shtylyov @ 2015-11-21 21:32 UTC (permalink / raw)
  To: Martin Blumenstingl, netdev
  Cc: willemb, edumazet, dborkman, davem, mostrows, nbd

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 <martin.blumenstingl@googlemail.com>

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3] packet: Allow packets with only a header (but no payload)
  2015-11-21  0:50 ` Martin Blumenstingl
  2015-11-21 21:32   ` Sergei Shtylyov
@ 2015-11-22 16:46   ` Martin Blumenstingl
  2015-11-30  3:18     ` David Miller
  1 sibling, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2015-11-22 16:46 UTC (permalink / raw)
  To: netdev
  Cc: willemb, edumazet, davem, nbd, sergei.shtylyov, Martin Blumenstingl

Commit 9c7077622dd91 ("packet: make packet_snd fail on len smaller
than l2 header") added validation for the packet size in packet_snd.
This change enforces that every packet needs a header (with at least
hard_header_len bytes) plus a payload with at least one byte. Before
this change the payload was optional.

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 <martin.blumenstingl@googlemail.com>
---
v3: Fixed a whitespace error (detected by checkpatch.pl) and updated
    the commit message to point to the original commit correctly.
    Thanks to Sergei Shtylyov for reporting these.

v2: Simply change the existing logic in ll_header_truncated instead of
    splitting it and having multiple checks.

 include/linux/netdevice.h | 3 ++-
 net/packet/af_packet.c    | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 67bfac1..9a488ed 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1398,7 +1398,8 @@ enum netdev_priv_flags {
  *	@dma:		DMA channel
  *	@mtu:		Interface MTU value
  *	@type:		Interface hardware type
- *	@hard_header_len: Hardware header length
+ *	@hard_header_len: Hardware header length, which means that this is the
+ *				minimum size of a packet.
  *
  *	@needed_headroom: Extra headroom the hardware may need, but not in all
  *			  cases can this be guaranteed
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1cf928f..992396a 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2329,8 +2329,8 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 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)) {
-		net_warn_ratelimited("%s: packet size is too short (%d <= %d)\n",
+	if (unlikely(len < dev->hard_header_len)) {
+		net_warn_ratelimited("%s: packet size is too short (%d < %d)\n",
 				     current->comm, len, dev->hard_header_len);
 		return true;
 	}
-- 
2.6.2

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3] packet: Allow packets with only a header (but no payload)
  2015-11-22 16:46   ` [PATCH v3] " Martin Blumenstingl
@ 2015-11-30  3:18     ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2015-11-30  3:18 UTC (permalink / raw)
  To: martin.blumenstingl; +Cc: netdev, willemb, edumazet, nbd, sergei.shtylyov

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Sun, 22 Nov 2015 17:46:09 +0100

> Commit 9c7077622dd91 ("packet: make packet_snd fail on len smaller
> than l2 header") added validation for the packet size in packet_snd.
> This change enforces that every packet needs a header (with at least
> hard_header_len bytes) plus a payload with at least one byte. Before
> this change the payload was optional.
> 
> 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 <martin.blumenstingl@googlemail.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-11-30  3:18 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-21 16:14 [PATCH] packet: Allow packets with only a header (but no payload) Martin Blumenstingl
2015-07-21 16:28 ` Willem de Bruijn
2015-07-21 16:38   ` Martin Blumenstingl
2015-07-21 16:51     ` Willem de Bruijn
2015-07-27 22:35       ` Martin Blumenstingl
2015-07-29  6:05         ` Willem de Bruijn
2015-07-30 22:15           ` Martin Blumenstingl
2015-11-07 13:11             ` Felix Fietkau
2015-11-09 17:53               ` Willem de Bruijn
2015-11-09 19:02                 ` Felix Fietkau
2015-11-21  0:50 ` Martin Blumenstingl
2015-11-21 21:32   ` Sergei Shtylyov
2015-11-22 16:46   ` [PATCH v3] " Martin Blumenstingl
2015-11-30  3:18     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).