netdev - Re: [RFC] move dma_head/dma_maps out of skb_shared_info and into sk

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]

Message-Id: <1257477847.14523.30.camel@localhost>
Date:	Thu, 05 Nov 2009 19:24:07 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Alexander Duyck <alexander.h.duyck@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [RFC] move dma_head/dma_maps out of skb_shared_info and into
 sk_buff

On Fri, 2009-11-06 at 03:14 +0100, Eric Dumazet wrote:
> Alexander Duyck a écrit :
> > During testing we found issues with the use of skb_dma_map/unmap on
> > systems that had iommu enabled and were configured to use a bridge.  The
> > issue is that if two ports are members of the same bridge, and a
> > broadcast packet is sent out on the bridge skb_clone will be used to
> > send a copy to all ports, but the clones run into issues because the
> > dma mappings for the cloned skbs all share the shared_info structure
> > where the dma mappings are stored.
> > 
> > To resolve that this patch moves those dma mappings out of the
> > shared_info structure and into the sk_buff itself.  This allows cloned
> > skbs to be mapped separately without causing dma unmapping errors.
> > 
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
> 
> Hello Alexander
> 
> You probably know such a change is a major one ;)
> 1) a diffstat -p1 -w70 for this kind of patch would be nice.
> 
> 2) Your patch is garbled (tabulations were replaced by spaces
>  by your mailer)

I kind of figured that might be the case.  I didn't really intend the
patch to be applied to the tree and just meant it to get conversation
going.  That is why I had tagged it as [RFC].

> 3) Are you sure we need to clear dma_maps[] array and dma_head 
>    in __alloc_skb() ? I guess not.
>    MAX_SKB_FRAGS = 18 on x86  -> 152 bytes on x86_64.
>    Previous implementation was not clearing them.
>    Thats would be a major slow down.
> 
> 4) 152 bytes more in skb -> 304 bytes more in skbuff_fclone_cache
>    Do we really want two copies of dma_maps[] when skb are allocated
>    from fclone cache ?

The main problem that this was meant to address is the fact that
skb_dma_map is called dma_maps and dma_head needed to be maintained
until skb_dma_unmap was called.  This wasn't happening with them being
stored in the skb_shared_info structure due to the fact that if two
clones of the skb were mapped on 2 different devices the 2nd mapping
would overwrite the first, and then the skb_dma_unmap call was being
called on the 2nd dma mapping twice which would trigger a dma_unmapping
error followed by an error of mappings still being held for the first
device on driver unload.

> 5) It seems to me this stuff is needed for xmit only and few drivers,
>    could we find a way to not have it for RX path and drivers that dont
>    need it ? Maybe drivers themselves should allocate storage for this
>   stuff so we can remove it both from shared_info *and* skb 

I'm thinking the best solution may be to drop the skb_dma_map/unmap
calls entirely and move things back to the old approach in which devices
maintained their list of mappings.  At least until something better can
be figured out.

Thanks,

Alex


--
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