NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

The current value doesn't make much sense anymore because skb_shared_info isn't taken into account which means that depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE resulting in a waste of almost a complete page.

Using SKB_MAXORDER solves this potential leak at the cost of slightly smaller but safer sizes for some architectures.

Quoted 5 lines · Thomas Graf

The current value doesn't make much sense anymore because skb_shared_info isn't taken into account which means that depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE resulting in a waste of almost a complete page.

You're quite right.

Quoted 2 lines

Using SKB_MAXORDER solves this potential leak at the cost of slightly smaller but safer sizes for some architectures.

At first glance it's not clear which of sk_buff or skb_shared_info is bigger. So it might even end up being bigger :)

include/linux/netlink.h line 138

Quoted 3 lines

include/linux/netlink.h +2 -1

-#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))
+#define NLMSG_GOODORDER 0
+#define NLMSG_GOODSIZE (SKB_MAX_ORDER(0, NLMSG_GOODORDER))

Are we ever going use NLMSG_GOODORDER for anything? If not why don't we go straight to NLMSG_GOODSIZE?

Quoted 5 lines

Quoted 2 lines

Using SKB_MAXORDER solves this potential leak at the cost of slightly smaller but safer sizes for some architectures.

At first glance it's not clear which of sk_buff or skb_shared_info is bigger. So it might even end up being bigger :)

skb_shared_info is 160 bytes and sk_buff is 196 bytes on my box (x86) but that's not the point. We need to take SMP_CACHE_BYTES alignment into account which tends to be quite big. The original NLMSG_GOODSIZE results in 3908 on my box and gets pumped up to 4128 by skb_alloc (ALIGN(...,SMP_CACHE_BYTES) + sizeof(struct skb_shared_info)) having SMP_CACHE_BYTES at 128.

Quoted 1 line

-#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))

Quoted 1 line

Quoted 1 line

-#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))

... which reminded me about an old stale patchlet:

Comment: improper size calculation for collapsed skb size.
Noticed by Denis V. Lunev <[email protected]>

===== net/ipv4/tcp_input.c 1.89 vs edited =====

* Thomas Graf <[email protected]> 2005-01-29 01:21

Quoted 10 lines

net/ipv4/tcp_input.c +1 -2

@@ -3760,8 +3760,7 @@
 	while (before(start, end)) {
 		struct sk_buff *nskb;
 		int header = skb_headroom(skb);
-		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
-			    sizeof(struct skb_shared_info) - header - 31)&~15;
+		int copy = SKB_MAX_ORDER(header + 31, 0);
 

net/ipv4/tcp_input.c line 3766

Quoted 2 lines

net/ipv4/tcp_input.c +0 -0

 		/* Too big header? This can happen with IPv6. */
 		if (copy < 0)

Sorry, this is incomplete, we should refetch copy via (skb->end - skb->head) after allocating it. I have to think some more about this first. ;-)

Quoted 6 lines

Quoted 6 lines

net/ipv4/tcp_input.c +1 -2

 		struct sk_buff *nskb;
 		int header = skb_headroom(skb);
-		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
-			    sizeof(struct skb_shared_info) - header - 31)&~15;
+		int copy = SKB_MAX_ORDER(header + 31, 0);

Quoted 5 lines

net/ipv4/tcp_input.c line 3766

Quoted 2 lines

net/ipv4/tcp_input.c +0 -0

 		/* Too big header? This can happen with IPv6. */
 		if (copy < 0)

Sorry, this is incomplete, we should refetch copy via (skb->end - skb->head) after allocating it. I have to think some more about this first. ;-)

I don't understand, if we alloc_skb(copy) we are guarenteed to have "copy" bytes available in the SKB data area.

patch also applies to 2.4 with some fuzz.

Noticed by Denis V. Lunev <[email protected]>

Signed-off-by Thomas Graf <[email protected]>

alignment though. Can someone enlighten me about the magic 31 in there?

NLMSG_GOODSIZE specifies a good default size for the skb tailroom used in netlink messages when the size is unknown at the time of the allocation.

The current value doesn't make much sense anymore because skb_shared_info isn't taken into account which means that depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE resulting in a waste of almost a complete page.

Using SKB_MAXORDER solves this potential leak at the cost of slightly smaller but safer sizes for some architectures.

Signed-off-by Thomas Graf <[email protected]>

Dave, did you get this one?

NLMSG_GOODSIZE specifies a good default size for the skb tailroom used in netlink messages when the size is unknown at the time of the allocation.

The current value doesn't make much sense anymore because skb_shared_info isn't taken into account which means that depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE resulting in a waste of almost a complete page.

Using SKB_MAXORDER solves this potential leak at the cost of slightly smaller but safer sizes for some architectures.