netdev - Re: [PATCH net] bna: page allocation during interrupts to use a mempool.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 06 Oct 2014 20:15:46 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Eric Wheeler <netdev@...ts.ewheeler.net>
Cc:	Shahed Shaikh <shahed.shaikh@...gic.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	netdev <netdev@...r.kernel.org>,
	Rasesh Mody <rasesh.mody@...gic.com>
Subject: Re: [PATCH net] bna: page allocation during interrupts to use a
 mempool.

On Mon, 2014-10-06 at 20:05 -0700, Eric Wheeler wrote:
> On Mon, 6 Oct 2014, Eric Dumazet wrote:
> > On Mon, 2014-10-06 at 18:57 -0700, Eric Wheeler wrote:
> >> This patch fixes an order:2 memory allocation error backtrace by
> >> guaranteeing that memory is available during simultaneous high memory
> >> pressure and packet rates when using 9k jumbo frames.
> >
> > This is highly suspect to me.
> > Most likely yet another truesize lie.
> > At a first glance, bnad_cq_setup_skb_frags() is buggy here :
> > skb->truesize += totlen;
> 
> skb->truesize wasn't part of my patch, can you explain in more detail what 
> you suggest a better fix might be? If you write a quick patch I can test 
> it.
> 
> The patch in question implements a simple mempool for the interrupt page 
> allocs---which definitely fixes the problem even if a better solution 
> might exist.  I have no problem giving up a small amount of memory to 
> guarantee page allocs in the interrupt handler.
> 
> It would be great to see this patch pushed through since it does fix the 
> problem---at least until we can come up with a better fix.  I'm happy to 
> test if you can send a patch.

It seems many drivers make this assumption that a frame of 1000 bytes
consumes 1000 bytes of memory.

Reality is that driver allocated more memory, because it can not predict
how many bytes are going to be received from the network.

By lying on skb->truesize (underestimating real memory cost), this
prevents networking stack making appropriate memory checks.

Here, it seems clear to me the following fix is needed, at very minimum.

And it might be that something better is needed :  MTU=9000 might force
the driver to allocate 16384 bytes per frame, not 9018

So it is possible that unmap->vector.len needs to be changed to the real
size of memory region (for example : PAGE_SIZE << 2)


diff --git a/drivers/net/ethernet/brocade/bna/bnad.c b/drivers/net/ethernet/brocade/bna/bnad.c
index ffc92a41d75be550d27698af6ca3e600d9a146fe..ce867219e2ceaf33b17595b67bae99e964d5a6b6 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -550,6 +550,7 @@ bnad_cq_setup_skb_frags(struct bna_rcb *rcb, struct sk_buff *skb,
 				dma_unmap_addr(&unmap->vector, dma_addr),
 				unmap->vector.len, DMA_FROM_DEVICE);
 
+		skb->truesize += unmap->vector.len;
 		len = (vec == nvecs) ?
 			last_fraglen : unmap->vector.len;
 		totlen += len;
@@ -563,7 +564,6 @@ bnad_cq_setup_skb_frags(struct bna_rcb *rcb, struct sk_buff *skb,
 
 	skb->len += totlen;
 	skb->data_len += totlen;
-	skb->truesize += totlen;
 }
 
 static inline void



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html