Re: [PATCH net-next 08/13] net/mlx5e: Add fragmented memory support for RX multi packet WQE

On ven., 2016-03-11 at 21:25 +0200, Saeed Mahameed wrote:

quoted

quoted

-void mlx5e_handle_rx_cqe_mpwrq(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
+static void mlx5e_add_skb_frag(struct sk_buff *skb, int len, struct page *page,
+                            int page_offset)
+{
+     int f = skb_shinfo(skb)->nr_frags++;
+     skb_frag_t *fr = &skb_shinfo(skb)->frags[f];
+
+     skb->len += len;
+     skb->data_len += len;
+     get_page(page);
+     skb_frag_set_page(skb, f, page);
+     skb_frag_size_set(fr, len);
+     fr->page_offset = page_offset;
+     skb->truesize  = SKB_TRUESIZE(skb->len);
+}

Really I am speechless.

It is hard to believe how much effort some drivers authors spend trying
to fool linux stack and risk OOM a host under stress.

Eric, you got it all wrong my friend, no one is trying to fool nobody here.
I will explain it to you below.

quoted

SKB_TRUESIZE() is absolutely not something a driver is allowed to use.

Here you want instead :

skb->truesize += PAGE_SIZE;

Assuming you allocate and use an order-0 page per fragment. Fact that
you receive say 100 bytes datagram is irrelevant to truesize.

Your assumption is wrong, we allocate as many pages as a WQE needs,
and a WQE can describe/handle
up to 1024 packets which share the same page/pages, so the skb should
really have a true size of the strides
of that page it used and not the WHOLE page as you think.

you should already learn this from the previous patch.

each WQE (Receive Work Queue Element) contains 1024 strides each of
the size 128B,
i.e, a packet of the size 128B or less will consume only one stride of
that WQE page, next packets on that WQE
will use the following strides in that same page.

So in opposite of what you think this new scheme is better than our
old one in terms of memory utilization.
before, we wasted MTU size per SKB/Packet regardless of the real
packet size, now each SKB will consume only
as much as 128B strides it will need, no more no less.

BTW there will be only 16 WQEs per ring :), so this new approach
doesn't drastically consume more memory than the previous one.
But it sure can handle more small packets bursts.

quoted

truesize is the real memory usage of one skb. Not the minimal size of an
optimally allocated skb for a given payload.

I totally agree with this, we should have reported  skb->truesize +=
(consumed strides)*(stride size).
but again this is not as critical as you think, in the worst case
skb->truesize will be off by 127B at most.

Ouch. really you are completely wrong.

If one skb has a fragment of a page, and sits in a queue for a long
time, it really uses a full page, because the remaining part of the page
is not reusable. Only kmalloc(128) can deal with that idea of allowing
other parts of the page being 'freed and reusable'

It is trivial for an attacker to make sure the host will consume one
page + sk_buff + skb->head = 4096 + 256 + 512, by specially sending out
of order packets on TCP flows.

It is very tempting to have special memory allocators you know, but you
have to understand attackers are smart. Smarter than us.

If now you are telling me you plan to allocate 131072 bytes pages (1024
strides of 128 bytes), then a smart attacker can actually bump skb
truesize to 128KB

Really your RX allocation schem is easily DOSable.