* [PATCH] net: move padding in struct skb_shared_info
@ 2017-04-10 8:07 Alexey Dobriyan
2017-04-10 14:43 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Alexey Dobriyan @ 2017-04-10 8:07 UTC (permalink / raw)
To: davem; +Cc: steffen.klassert, edumazet, netdev
commit 7f564528a480084e2318cd48caba7aef4a54a77f
("skbuff: Extend gso_type to unsigned int.") created padding as first
field of struct skb_shared_info requiring [R64+imm8] addressing mode
for all fields.
Patch bubbles up padding brinding code size down to original levels and
even smaller:
add/remove: 0/0 grow/shrink: 4/304 up/down: 20/-720 (-700)
function old new delta
iwl_trans_pcie_tx 3994 4006 +12
tap_do_read 1070 1074 +4
packet_recvmsg 1155 1157 +2
be_xmit 2038 2040 +2
zerocopy_sg_from_iter 455 454 -1
...
__ef4_rx_packet 1358 1349 -9
hix5hd2_poll 1787 1777 -10
e1000_clean_jumbo_rx_irq 3599 3587 -12
skb_try_coalesce 1118 1105 -13
xenvif_tx_build_gops 5057 5043 -14
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
include/linux/skbuff.h | 1 -
1 file changed, 1 deletion(-)
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -413,7 +413,6 @@ struct ubuf_info {
* the end of the header data, ie. at skb->end.
*/
struct skb_shared_info {
- unsigned short _unused;
unsigned char nr_frags;
__u8 tx_flags;
unsigned short gso_size;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: move padding in struct skb_shared_info
2017-04-10 8:07 [PATCH] net: move padding in struct skb_shared_info Alexey Dobriyan
@ 2017-04-10 14:43 ` Eric Dumazet
2017-04-11 9:41 ` Alexey Dobriyan
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2017-04-10 14:43 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: davem, steffen.klassert, edumazet, netdev
On Mon, 2017-04-10 at 11:07 +0300, Alexey Dobriyan wrote:
> commit 7f564528a480084e2318cd48caba7aef4a54a77f
> ("skbuff: Extend gso_type to unsigned int.") created padding as first
> field of struct skb_shared_info requiring [R64+imm8] addressing mode
> for all fields.
>
> Patch bubbles up padding brinding code size down to original levels and
> even smaller:
>
> add/remove: 0/0 grow/shrink: 4/304 up/down: 20/-720 (-700)
> function old new delta
> iwl_trans_pcie_tx 3994 4006 +12
> tap_do_read 1070 1074 +4
> packet_recvmsg 1155 1157 +2
> be_xmit 2038 2040 +2
> zerocopy_sg_from_iter 455 454 -1
> ...
> __ef4_rx_packet 1358 1349 -9
> hix5hd2_poll 1787 1777 -10
> e1000_clean_jumbo_rx_irq 3599 3587 -12
> skb_try_coalesce 1118 1105 -13
> xenvif_tx_build_gops 5057 5043 -14
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> include/linux/skbuff.h | 1 -
> 1 file changed, 1 deletion(-)
>
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -413,7 +413,6 @@ struct ubuf_info {
> * the end of the header data, ie. at skb->end.
> */
> struct skb_shared_info {
> - unsigned short _unused;
> unsigned char nr_frags;
> __u8 tx_flags;
> unsigned short gso_size;
Nack
This exact placement was discussed at Netconf and Netdev.
We had off-by-one errors in the past leading to nr_frags being mangled,
and some exploits were quite happy to use these bugs.
Some shuffling in shared_info might help us to find other bugs, and give
more work to security researchers
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: move padding in struct skb_shared_info
2017-04-10 14:43 ` Eric Dumazet
@ 2017-04-11 9:41 ` Alexey Dobriyan
2017-04-11 14:47 ` Eric Dumazet
2017-04-11 14:52 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Alexey Dobriyan @ 2017-04-11 9:41 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Steffen Klassert, Eric Dumazet, netdev
On Mon, Apr 10, 2017 at 5:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2017-04-10 at 11:07 +0300, Alexey Dobriyan wrote:
>> struct skb_shared_info {
>> - unsigned short _unused;
>> unsigned char nr_frags;
>> __u8 tx_flags;
>> unsigned short gso_size;
>
> Nack
>
> This exact placement was discussed at Netconf and Netdev.
>
> We had off-by-one errors in the past leading to nr_frags being mangled,
> and some exploits were quite happy to use these bugs.
>
> Some shuffling in shared_info might help us to find other bugs, and give
> more work to security researchers
By this logic there should be redzone around every field in networking stack.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: move padding in struct skb_shared_info
2017-04-11 9:41 ` Alexey Dobriyan
@ 2017-04-11 14:47 ` Eric Dumazet
2017-04-11 14:52 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2017-04-11 14:47 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: David Miller, Steffen Klassert, Eric Dumazet, netdev
On Tue, 2017-04-11 at 12:41 +0300, Alexey Dobriyan wrote:
> On Mon, Apr 10, 2017 at 5:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Mon, 2017-04-10 at 11:07 +0300, Alexey Dobriyan wrote:
> >> struct skb_shared_info {
> >> - unsigned short _unused;
> >> unsigned char nr_frags;
> >> __u8 tx_flags;
> >> unsigned short gso_size;
> >
> > Nack
> >
> > This exact placement was discussed at Netconf and Netdev.
> >
> > We had off-by-one errors in the past leading to nr_frags being mangled,
> > and some exploits were quite happy to use these bugs.
> >
> > Some shuffling in shared_info might help us to find other bugs, and give
> > more work to security researchers
>
> By this logic there should be redzone around every field in networking stack.
Only shared_info immediately follows skb->head
For other structures, standard debug tools in SLUB/SLB/KASAN already
take care of redzones.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] net: move padding in struct skb_shared_info
2017-04-11 9:41 ` Alexey Dobriyan
2017-04-11 14:47 ` Eric Dumazet
@ 2017-04-11 14:52 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-04-11 14:52 UTC (permalink / raw)
To: adobriyan; +Cc: eric.dumazet, steffen.klassert, edumazet, netdev
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Tue, 11 Apr 2017 12:41:08 +0300
> On Mon, Apr 10, 2017 at 5:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Mon, 2017-04-10 at 11:07 +0300, Alexey Dobriyan wrote:
>>> struct skb_shared_info {
>>> - unsigned short _unused;
>>> unsigned char nr_frags;
>>> __u8 tx_flags;
>>> unsigned short gso_size;
>>
>> Nack
>>
>> This exact placement was discussed at Netconf and Netdev.
>>
>> We had off-by-one errors in the past leading to nr_frags being mangled,
>> and some exploits were quite happy to use these bugs.
>>
>> Some shuffling in shared_info might help us to find other bugs, and give
>> more work to security researchers
>
> By this logic there should be redzone around every field in networking stack.
It is not a general rule, but a situation we specifically know about
here for this structure and how exploits are actively being coded.
I would like to politely ask that we don't strive so strongly for
"tree wide rules" that we fail to see the need for specific handling
in specific situations as is the case here.
Thank you.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-11 14:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 8:07 [PATCH] net: move padding in struct skb_shared_info Alexey Dobriyan
2017-04-10 14:43 ` Eric Dumazet
2017-04-11 9:41 ` Alexey Dobriyan
2017-04-11 14:47 ` Eric Dumazet
2017-04-11 14:52 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.