Misalignment, MIPS, and ip_hdr(skb)->version

Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

Hey MIPS Networking People,

I receive encrypted packets with a 13 byte header. I decrypt the
ciphertext in place, and then discard the header. I then pass the
plaintext to the rest of the networking stack. The plaintext is an IP
packet. Due to the 13 byte header that was discarded, the plaintext
possibly begins at an unaligned location (depending on whether
dev->needed_headroom was respected).

Does this matter? Is this bad? Will there be a necessary performance hit?

In order to find out, I instrumented the MIPS unaligned access
exception handler to see where I was actually in trouble.
Surprisingly, the only part of the stack that seemed to be upset was
on calls to ip_hdr(skb)->version.

Two things disturb me about this. First, this seems too good to be
true. Does it seem reasonable to you that this is actually the only
place that would be problematic? Or was my testing methodology wrong
to arrive at such an optimistic conclusion?

Secondly, why should a call to ip_hdr(skb)->version cause an unaligned
access anyway? This struct member is simply the second half of a
single byte in a bit field. I'd expect for the compiler to generate a
single byte load, followed by a bitshift or a mask. Instead, the
compiler appears to generate a double byte load, hence the exception.
What's up with this? Stupid compiler that should be fixed? Some odd
optimization? What to do?

I'm considering just adding an extra byte of padding (see discussion
in [1]), but before I make any decision like that (and hopefully it
won't be necessary), I'd like to completely and entirely understand
the full effects and consequences of calling netif_rx(skb) when
skb->data is unaligned. Any insight you have to offer would be most
welcome.

Thanks,
Jason

[1] https://lists.zx2c4.com/pipermail/wireguard/2016-December/000709.html

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Dave Taht]

The openwrt tree has long contained a set of patches that correct for
unaligned issues throughout the linux network stack.

https://git.lede-project.org/?p=openwrt/source.git;a=blob;f=target/linux/ar71xx/patches-4.4/910-unaligned_access_hacks.patch;h=b4b749e4b9c02a74a9f712a2740d63e554de5c64;hb=ee53a240ac902dc83209008a2671e7fdcf55957a

unaligned access traps in the packet processing path on certain versions of
the mips architecture is horrifically bad. I had kind of hoped these
patches in some form would have made it upstream by now. (or the
arches that have the issue retired, I think it's mostly just mips24k)

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[David Miller]

From: Dave Taht <dave.taht@gmail.com>
Date: Wed, 7 Dec 2016 10:47:16 -0800

> https://git.lede-project.org/?p=openwrt/source.git;a=blob;f=target/linux/ar71xx/patches-4.4/910-unaligned_access_hacks.patch;h=b4b749e4b9c02a74a9f712a2740d63e554de5c64;hb=ee53a240ac902dc83209008a2671e7fdcf55957a

It's so much better to analyze properly where the misalignment comes from
and address it at the source, as we have for various cases that trip up
Sparc too.

Marking structures "packed" is going to kill performance and is not
the answer.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
> It's so much better to analyze properly where the misalignment comes from
> and address it at the source, as we have for various cases that trip up
> Sparc too.

That's sort of my attitude too, hence starting this thread. Any
pointers you have about this would be most welcome, so as not to
perpetuate what already seems like an issue in other parts of the
stack.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Wed, 7 Dec 2016 19:54:12 +0100

> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>> It's so much better to analyze properly where the misalignment comes from
>> and address it at the source, as we have for various cases that trip up
>> Sparc too.
> 
> That's sort of my attitude too, hence starting this thread. Any
> pointers you have about this would be most welcome, so as not to
> perpetuate what already seems like an issue in other parts of the
> stack.

The only truly difficult case to handle is GRE encapsulation.  Is
that the situation you are running into?

If not, please figure out what the header configuration looks like
in the case that hits for you, and what the originating device is
just in case it is a device driver issue.

Thanks.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

On Wed, Dec 7, 2016 at 8:52 PM, David Miller <davem@davemloft.net> wrote:
> The only truly difficult case to handle is GRE encapsulation.  Is
> that the situation you are running into?
>
> If not, please figure out what the header configuration looks like
> in the case that hits for you, and what the originating device is
> just in case it is a device driver issue.

My case is my own driver and my own protocol, which uses a 13 byte
header. I can, if absolutely necessary, change the protocol to add
another byte of padding. Or I can choose not to decrypt in place but
rather use a different trick, like overwriting the header during
decryption, though this removes some of the scatterwalk optimizations
when src and dst are the same. Or something else. I wrote the top
email of this thread inquiring about just exactly how bad it is to
call netif_rx(skb) when skb->data is unaligned.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Hannes Frederic Sowa]

Hi Jason,

On 07.12.2016 19:35, Jason A. Donenfeld wrote:
> I receive encrypted packets with a 13 byte header. I decrypt the
> ciphertext in place, and then discard the header. I then pass the
> plaintext to the rest of the networking stack. The plaintext is an IP
> packet. Due to the 13 byte header that was discarded, the plaintext
> possibly begins at an unaligned location (depending on whether
> dev->needed_headroom was respected).
> 
> Does this matter? Is this bad? Will there be a necessary performance hit?

Your custom protocol should be designed in a way you get an aligned ip
header. Most protocols of the IETF follow this mantra and it is always
possible to e.g. pad options so you end up on aligned boundaries for the
next header.

GRE-TEB for example needs skb_copy_bits to extract the header so it can
access them in an aligned way.

> In order to find out, I instrumented the MIPS unaligned access
> exception handler to see where I was actually in trouble.
> Surprisingly, the only part of the stack that seemed to be upset was
> on calls to ip_hdr(skb)->version.
> 
> Two things disturb me about this. First, this seems too good to be
> true. Does it seem reasonable to you that this is actually the only
> place that would be problematic? Or was my testing methodology wrong
> to arrive at such an optimistic conclusion?
> 
> Secondly, why should a call to ip_hdr(skb)->version cause an unaligned
> access anyway? This struct member is simply the second half of a
> single byte in a bit field. I'd expect for the compiler to generate a
> single byte load, followed by a bitshift or a mask. Instead, the
> compiler appears to generate a double byte load, hence the exception.
> What's up with this? Stupid compiler that should be fixed? Some odd
> optimization? What to do?

I don't see an issue with that at all. Why do you think it could be a
problem?

Bye,
Hannes

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Thu, 8 Dec 2016 01:29:42 +0100

> On Wed, Dec 7, 2016 at 8:52 PM, David Miller <davem@davemloft.net> wrote:
>> The only truly difficult case to handle is GRE encapsulation.  Is
>> that the situation you are running into?
>>
>> If not, please figure out what the header configuration looks like
>> in the case that hits for you, and what the originating device is
>> just in case it is a device driver issue.
> 
> My case is my own driver and my own protocol, which uses a 13 byte
> header. I can, if absolutely necessary, change the protocol to add
> another byte of padding. Or I can choose not to decrypt in place but
> rather use a different trick, like overwriting the header during
> decryption, though this removes some of the scatterwalk optimizations
> when src and dst are the same. Or something else. I wrote the top
> email of this thread inquiring about just exactly how bad it is to
> call netif_rx(skb) when skb->data is unaligned.

You really have to land the IP header on a proper 4 byte boundary.

I would suggest pushing 3 dummy garbage bytes of padding at the front
or the end of your header.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Daniel Kahn Gillmor]

On Wed 2016-12-07 19:30:34 -0500, Hannes Frederic Sowa wrote:
> Your custom protocol should be designed in a way you get an aligned ip
> header. Most protocols of the IETF follow this mantra and it is always
> possible to e.g. pad options so you end up on aligned boundaries for the
> next header.

fwiw, i'm not convinced that "most protocols of the IETF follow this
mantra".  we've had multiple discussions in different protocol groups
about shaving or bloating by a few bytes here or there in different
protocols, and i don't think anyone has brought up memory alignment as
an argument in any of the discussions i've followed.

that said, it sure does sound like it would make things simpler to
construct the protocol that way :)

          --dkg

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

Hi David,

On Thu, Dec 8, 2016 at 1:37 AM, David Miller <davem@davemloft.net> wrote:
> You really have to land the IP header on a proper 4 byte boundary.
>
> I would suggest pushing 3 dummy garbage bytes of padding at the front
> or the end of your header.

Are you sure 3 bytes to get 4 byte alignment is really the best? I was
thinking that adding 1 byte to get 2 byte alignment might be better,
since it would then ensure that the subsequent TCP header winds up
being 4 byte aligned. Or is this in fact not the desired trade off,
and so I should stick with the 3 bytes you suggested?

Jason

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Thu, 8 Dec 2016 23:20:04 +0100

> Hi David,
> 
> On Thu, Dec 8, 2016 at 1:37 AM, David Miller <davem@davemloft.net> wrote:
>> You really have to land the IP header on a proper 4 byte boundary.
>>
>> I would suggest pushing 3 dummy garbage bytes of padding at the front
>> or the end of your header.
> 
> Are you sure 3 bytes to get 4 byte alignment is really the best? I was
> thinking that adding 1 byte to get 2 byte alignment might be better,
> since it would then ensure that the subsequent TCP header winds up
> being 4 byte aligned. Or is this in fact not the desired trade off,
> and so I should stick with the 3 bytes you suggested?

If the IP header is 4 byte aligned, the TCP header will be as well.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jiri Benc]

On Wed, 07 Dec 2016 23:34:21 -0500, Daniel Kahn Gillmor wrote:
> fwiw, i'm not convinced that "most protocols of the IETF follow this
> mantra".  we've had multiple discussions in different protocol groups
> about shaving or bloating by a few bytes here or there in different
> protocols, and i don't think anyone has brought up memory alignment as
> an argument in any of the discussions i've followed.

Which is sad. One would expect that this would be well understood for
decades already.

 Jiri

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-07 19:54, Jason A. Donenfeld wrote:
> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>> It's so much better to analyze properly where the misalignment comes from
>> and address it at the source, as we have for various cases that trip up
>> Sparc too.
> 
> That's sort of my attitude too, hence starting this thread. Any
> pointers you have about this would be most welcome, so as not to
> perpetuate what already seems like an issue in other parts of the
> stack.
Hi Jason,

I'm the author of that hackish LEDE/OpenWrt patch that works around the
misalignment issues. Here's some context regarding that patch:

I intentionally put it in the target specific patches for only one of
our MIPS targets. There are a few ar71xx devices where the misalignment
cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
requirement, and does not support inserting 2 bytes of padding to
correct the IP header misalignment.

With these limitations the choice was between this ugly network stack
patch or inserting a very expensive memmove in the data path (which is
better than taking the mis-alignment traps, but still hurts routing
performance significantly).

There are a lot of places in the network stack that assume full 32 bit
alignment, and you only get to see those once you start using more of
netfilter, play with various tunnel encapsulations, etc.

I think you have 3 options to deal with this properly:
1. add 3 bytes of padding
2. allocate a separate skb for decryption (might be more expensive)
3. save the header and decrypt to the start of the packet data
(overwriting the misaligned header).

I'm not sure what the performance impact of 2 and 3 is, so it's probably
best to stick with the padding.

I've taken a quick look at the wireguard message headers, and my
recommendation would be to insert the 3-byte padding in struct
message_header and remove __packed from your structs.
This will also remove misaligment of your own protocol fields.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Måns Rullgård]

Felix Fietkau <nbd@nbd.name> writes:

> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>>> It's so much better to analyze properly where the misalignment comes from
>>> and address it at the source, as we have for various cases that trip up
>>> Sparc too.
>> 
>> That's sort of my attitude too, hence starting this thread. Any
>> pointers you have about this would be most welcome, so as not to
>> perpetuate what already seems like an issue in other parts of the
>> stack.
> Hi Jason,
>
> I'm the author of that hackish LEDE/OpenWrt patch that works around the
> misalignment issues. Here's some context regarding that patch:
>
> I intentionally put it in the target specific patches for only one of
> our MIPS targets. There are a few ar71xx devices where the misalignment
> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
> requirement, and does not support inserting 2 bytes of padding to
> correct the IP header misalignment.
>
> With these limitations the choice was between this ugly network stack
> patch or inserting a very expensive memmove in the data path (which is
> better than taking the mis-alignment traps, but still hurts routing
> performance significantly).

I solved this problem in an Ethernet driver by copying the initial part
of the packet to an aligned skb and appending the remainder using
skb_add_rx_frag().  The kernel network stack only cares about the
headers, so the alignment of the packet payload doesn't matter.

-- 
Måns Rullgård

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 14:25, Måns Rullgård wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>>>> It's so much better to analyze properly where the misalignment comes from
>>>> and address it at the source, as we have for various cases that trip up
>>>> Sparc too.
>>> 
>>> That's sort of my attitude too, hence starting this thread. Any
>>> pointers you have about this would be most welcome, so as not to
>>> perpetuate what already seems like an issue in other parts of the
>>> stack.
>> Hi Jason,
>>
>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>> misalignment issues. Here's some context regarding that patch:
>>
>> I intentionally put it in the target specific patches for only one of
>> our MIPS targets. There are a few ar71xx devices where the misalignment
>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>> requirement, and does not support inserting 2 bytes of padding to
>> correct the IP header misalignment.
>>
>> With these limitations the choice was between this ugly network stack
>> patch or inserting a very expensive memmove in the data path (which is
>> better than taking the mis-alignment traps, but still hurts routing
>> performance significantly).
> 
> I solved this problem in an Ethernet driver by copying the initial part
> of the packet to an aligned skb and appending the remainder using
> skb_add_rx_frag().  The kernel network stack only cares about the
> headers, so the alignment of the packet payload doesn't matter.
I considered that as well, but it's bad for routing performance if the
ethernet MAC does not support scatter/gather for xmit.
Unfortunately that limitation is quite common on embedded hardware.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Måns Rullgård]

Felix Fietkau <nbd@nbd.name> writes:

> On 2016-12-10 14:25, Måns Rullgård wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>>>>> It's so much better to analyze properly where the misalignment comes from
>>>>> and address it at the source, as we have for various cases that trip up
>>>>> Sparc too.
>>>> 
>>>> That's sort of my attitude too, hence starting this thread. Any
>>>> pointers you have about this would be most welcome, so as not to
>>>> perpetuate what already seems like an issue in other parts of the
>>>> stack.
>>> Hi Jason,
>>>
>>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>>> misalignment issues. Here's some context regarding that patch:
>>>
>>> I intentionally put it in the target specific patches for only one of
>>> our MIPS targets. There are a few ar71xx devices where the misalignment
>>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>>> requirement, and does not support inserting 2 bytes of padding to
>>> correct the IP header misalignment.
>>>
>>> With these limitations the choice was between this ugly network stack
>>> patch or inserting a very expensive memmove in the data path (which is
>>> better than taking the mis-alignment traps, but still hurts routing
>>> performance significantly).
>> 
>> I solved this problem in an Ethernet driver by copying the initial part
>> of the packet to an aligned skb and appending the remainder using
>> skb_add_rx_frag().  The kernel network stack only cares about the
>> headers, so the alignment of the packet payload doesn't matter.
>
> I considered that as well, but it's bad for routing performance if the
> ethernet MAC does not support scatter/gather for xmit.
> Unfortunately that limitation is quite common on embedded hardware.

Yes, I can see that being an issue.  However, if you're doing zero-copy
routing, the header part of the original buffer should still be there,
unused, so you could presumably copy the header of the outgoing packet
there and then do dma as usual.  Maybe there's something in the network
stack that makes this impossible though.

-- 
Måns Rullgård

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 21:32, Måns Rullgård wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> On 2016-12-10 14:25, Måns Rullgård wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>> 
>>>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <davem@davemloft.net> wrote:
>>>>>> It's so much better to analyze properly where the misalignment comes from
>>>>>> and address it at the source, as we have for various cases that trip up
>>>>>> Sparc too.
>>>>> 
>>>>> That's sort of my attitude too, hence starting this thread. Any
>>>>> pointers you have about this would be most welcome, so as not to
>>>>> perpetuate what already seems like an issue in other parts of the
>>>>> stack.
>>>> Hi Jason,
>>>>
>>>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>>>> misalignment issues. Here's some context regarding that patch:
>>>>
>>>> I intentionally put it in the target specific patches for only one of
>>>> our MIPS targets. There are a few ar71xx devices where the misalignment
>>>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>>>> requirement, and does not support inserting 2 bytes of padding to
>>>> correct the IP header misalignment.
>>>>
>>>> With these limitations the choice was between this ugly network stack
>>>> patch or inserting a very expensive memmove in the data path (which is
>>>> better than taking the mis-alignment traps, but still hurts routing
>>>> performance significantly).
>>> 
>>> I solved this problem in an Ethernet driver by copying the initial part
>>> of the packet to an aligned skb and appending the remainder using
>>> skb_add_rx_frag().  The kernel network stack only cares about the
>>> headers, so the alignment of the packet payload doesn't matter.
>>
>> I considered that as well, but it's bad for routing performance if the
>> ethernet MAC does not support scatter/gather for xmit.
>> Unfortunately that limitation is quite common on embedded hardware.
> 
> Yes, I can see that being an issue.  However, if you're doing zero-copy
> routing, the header part of the original buffer should still be there,
> unused, so you could presumably copy the header of the outgoing packet
> there and then do dma as usual.  Maybe there's something in the network
> stack that makes this impossible though.
That still puts more pressure on the ridiculously small dcache sizes
that are typical for embedded MIPS routers.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Dan Lüdtke]

> On 8 Dec 2016, at 05:34, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> 
> On Wed 2016-12-07 19:30:34 -0500, Hannes Frederic Sowa wrote:
>> Your custom protocol should be designed in a way you get an aligned ip
>> header. Most protocols of the IETF follow this mantra and it is always
>> possible to e.g. pad options so you end up on aligned boundaries for the
>> next header.
> 
> fwiw, i'm not convinced that "most protocols of the IETF follow this
> mantra".  we've had multiple discussions in different protocol groups
> about shaving or bloating by a few bytes here or there in different
> protocols, and i don't think anyone has brought up memory alignment as
> an argument in any of the discussions i've followed.
> 

If the trade-off is between 1 padding byte and 2 byte alignment versus 3 padding bytes and 4 byte alignment I would definitely opt for 3 padding bytes. I know how that waste feels like to a protocol designer, but I think it is worth it. Maybe the padding/reserved will be useful some day for an additional feature.

I remember alignment being discussed and taken very seriously in 6man a couple of times. Often, though, protocol designers did align without much discussion. Implementing unaligned protocols is a pain I've experienced first hand.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Greg KH]

On Sat, Dec 10, 2016 at 11:18:14PM +0100, Dan Lüdtke wrote:
> 
> > On 8 Dec 2016, at 05:34, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> > 
> > On Wed 2016-12-07 19:30:34 -0500, Hannes Frederic Sowa wrote:
> >> Your custom protocol should be designed in a way you get an aligned ip
> >> header. Most protocols of the IETF follow this mantra and it is always
> >> possible to e.g. pad options so you end up on aligned boundaries for the
> >> next header.
> > 
> > fwiw, i'm not convinced that "most protocols of the IETF follow this
> > mantra".  we've had multiple discussions in different protocol groups
> > about shaving or bloating by a few bytes here or there in different
> > protocols, and i don't think anyone has brought up memory alignment as
> > an argument in any of the discussions i've followed.
> > 
> 
> If the trade-off is between 1 padding byte and 2 byte alignment versus
> 3 padding bytes and 4 byte alignment I would definitely opt for 3
> padding bytes. I know how that waste feels like to a protocol
> designer, but I think it is worth it. Maybe the padding/reserved will
> be useful some day for an additional feature.

Note, if you do do this (hint, I think it is a good idea), require that
these reserved/pad fields always set to 0 for now, so that no one puts
garbage in them and then if you later want to use them, it will be a
mess.

thanks,

greg k-h

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Willy Tarreau]

Hi Jason,

On Thu, Dec 08, 2016 at 11:20:04PM +0100, Jason A. Donenfeld wrote:
> Hi David,
> 
> On Thu, Dec 8, 2016 at 1:37 AM, David Miller <davem@davemloft.net> wrote:
> > You really have to land the IP header on a proper 4 byte boundary.
> >
> > I would suggest pushing 3 dummy garbage bytes of padding at the front
> > or the end of your header.
> 
> Are you sure 3 bytes to get 4 byte alignment is really the best?

It's always the best. However there's another option which should be
considered : maybe it's difficult but not impossible to move some bits
from the current protocol to remove one byte. That's not always easy,
and sometimes you cannot do it just for one bit. However after you run
through this exercise, if you notice there's really no way to shave
this extra byte, you'll realize there's no room left for future
extensions and you'll more easily accept to add 3 empty bytes for
this, typically protocol version, tags, qos or flagss that you'll be
happy to rely on for future versions of your protocol.

Also while it can feel like you're making your protocol less efficient,
keep in mind that these 3 bytes only matter for small packets, and
Ethernet already pads small frames to 64 bytes, so in practice any
IP packet smaller than 46 bytes will not bring any extra savings.

Best regards,
Willy

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Måns Rullgård]

Willy Tarreau <w@1wt.eu> writes:

> Hi Jason,
>
> On Thu, Dec 08, 2016 at 11:20:04PM +0100, Jason A. Donenfeld wrote:
>> Hi David,
>> 
>> On Thu, Dec 8, 2016 at 1:37 AM, David Miller <davem@davemloft.net> wrote:
>> > You really have to land the IP header on a proper 4 byte boundary.
>> >
>> > I would suggest pushing 3 dummy garbage bytes of padding at the front
>> > or the end of your header.
>> 
>> Are you sure 3 bytes to get 4 byte alignment is really the best?
>
> It's always the best. However there's another option which should be
> considered : maybe it's difficult but not impossible to move some bits
> from the current protocol to remove one byte. That's not always easy,
> and sometimes you cannot do it just for one bit. However after you run
> through this exercise, if you notice there's really no way to shave
> this extra byte, you'll realize there's no room left for future
> extensions and you'll more easily accept to add 3 empty bytes for
> this, typically protocol version, tags, qos or flagss that you'll be
> happy to rely on for future versions of your protocol.

Always include some way of extending the protocol in the future.  A
single bit is often enough.  Require a value of zero initially, then if
you ever want to change anything, setting it to one can indicate
whatever you want, including a complete redesign of the header.
Alternatively, a one-bit field can indicate the presence of an extended
header yet to be defined.  Then old software can still make sense of the
basic header.

-- 
Måns Rullgård

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

Hey guys,

Thanks for the extremely detailed answers. The main take-away from
this is that passing unaligned packets to the networking stack kills
kittens. So now it's a question of mitigation. I have three options:

1. Copy the plaintext to three bytes before the start of the cipher
text, overwriting parts of the header that aren't actually required.
Pros: no changes required, MTU stays small.
Cons: scatterwalk's fast paths aren't hit, which means two page table
mappings are taken instead of one. I have no idea if this actually
matters or will slow down anything relavent.

2. Add 3 bytes to the plaintext header, set to zero, marked for future use.
Pros: satisfies IETF mantras and makes unaligned in-place decryption
straightforward.
Cons: lowers MTU, additional unauthenticated cleartext bits in the
header are of limited utility in protocol.

3. Add 3 bytes of padding, set to zero, to the encrypted section just
before the IP header, marked for future use.
Pros: satisfies IETF mantras, can use those extra bits in the future
for interesting protocol extensions for authenticated peers.
Cons: lowers MTU, marginally more difficult to implement but still
probably just one or two lines of code.

Of these, I'm leaning toward (3).

Anyway, thanks a lot for the input. "Doing nothing" is no longer under
serious consideration, thanks to your messages.

Thanks,
Jason

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Andrew Lunn]

> 3. Add 3 bytes of padding, set to zero, to the encrypted section just
> before the IP header, marked for future use.
> Pros: satisfies IETF mantras, can use those extra bits in the future
> for interesting protocol extensions for authenticated peers.
> Cons: lowers MTU, marginally more difficult to implement but still
> probably just one or two lines of code.

I'm not a crypto expert, but does this not give you a helping hand in
breaking the crypto? You know the plain text value of these bytes, and
where they are in the encrypted text.

      Andrew

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Jason A. Donenfeld]

On Sun, Dec 11, 2016 at 4:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> I'm not a crypto expert, but does this not give you a helping hand in
> breaking the crypto? You know the plain text value of these bytes, and
> where they are in the encrypted text.

You also know with some probability that there's going to be an IP
header and a TCP header, each with predictable fields. Maybe you're
reasonably certain there's an HTTP header in there too. Gasp! But fear
not...

Symmetric ciphers are generally not considered secure if they fall to
what's called a "known plaintext attack". Fortunately, modern ciphers
like AES and ChaCha20 and most others that you're aware of are
generally believed to be secure against KPA.

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Willy Tarreau]

On Sun, Dec 11, 2016 at 03:50:31PM +0100, Jason A. Donenfeld wrote:
> 3. Add 3 bytes of padding, set to zero, to the encrypted section just
> before the IP header, marked for future use.
> Pros: satisfies IETF mantras, can use those extra bits in the future
> for interesting protocol extensions for authenticated peers.
> Cons: lowers MTU, marginally more difficult to implement but still
> probably just one or two lines of code.
> 
> Of these, I'm leaning toward (3).

Or 4) add one byte to the cleartext header for future use (mostly flags
maybe) and 2 bytes of padding to the encrypted header. This way you get
the following benefits :
  1) your encrypted text is at least 16-bit aligned, maybe it matters
     in your checksum computations on during decryption
  2) your MTU remains even, this is better for both ends
  3) you're free to add some bits either to the encrypted or the clear
     parts.

Just a suggestion :-)

Willy

RE: Misalignment, MIPS, and ip_hdr(skb)->version

[David Laight]

From: Måns Rullgård
> Sent: 10 December 2016 13:25
...
> I solved this problem in an Ethernet driver by copying the initial part
> of the packet to an aligned skb and appending the remainder using
> skb_add_rx_frag().  The kernel network stack only cares about the
> headers, so the alignment of the packet payload doesn't matter.

That rather depends on where the packet payload ends up.
It is likely that it will be copied to userspace (or maybe
into some aligned kernel buffer).
In which case you get an expensive misaligned copy.

Encapsulation protocols not using headers that are multiples
of 4 bytes is as stupid as ethernet hardware that can't place
the mac address on a 4n+2 boundary.
The latter is particularly stupid when it happens on embedded
silicon with a processor that can only do aligned memory accesses.
What do the hardware engineers think the ethernet interface will
be used for!

	David

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Måns Rullgård]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Måns Rullgård
>> Sent: 10 December 2016 13:25
> ...
>> I solved this problem in an Ethernet driver by copying the initial part
>> of the packet to an aligned skb and appending the remainder using
>> skb_add_rx_frag().  The kernel network stack only cares about the
>> headers, so the alignment of the packet payload doesn't matter.
>
> That rather depends on where the packet payload ends up.
> It is likely that it will be copied to userspace (or maybe
> into some aligned kernel buffer).
> In which case you get an expensive misaligned copy.

There's not much to be done about that.  The only other option is to
bypass DMA entirely, and that's sure to be even worse.

> What do the hardware engineers think the ethernet interface will
> be used for!

Ticking boxes in marketing material.

-- 
Måns Rullgård