netdev - Re: [PATCH 2/6] net: mvneta: move skb build after descriptors processing

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: <20200716124426.4f7c3a67@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 16 Jul 2020 12:44:26 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, bpf@...r.kernel.org,
        ilias.apalodimas@...aro.org, brouer@...hat.com,
        echaudro@...hat.com, sameehj@...zon.com
Subject: Re: [PATCH 2/6] net: mvneta: move skb build after descriptors
 processing

On Thu, 16 Jul 2020 21:12:51 +0200 Lorenzo Bianconi wrote:
> > > +static struct sk_buff *
> > > +mvneta_swbm_build_skb(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> > > +		      struct xdp_buff *xdp, u32 desc_status)
> > > +{
> > > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > > +	int i, num_frags = sinfo->nr_frags;
> > > +	skb_frag_t frags[MAX_SKB_FRAGS];
> > > +	struct sk_buff *skb;
> > > +
> > > +	memcpy(frags, sinfo->frags, sizeof(skb_frag_t) * num_frags);
> > > +
> > > +	skb = build_skb(xdp->data_hard_start, PAGE_SIZE);
> > > +	if (!skb)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	page_pool_release_page(rxq->page_pool, virt_to_page(xdp->data));
> > > +
> > > +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> > > +	skb_put(skb, xdp->data_end - xdp->data);
> > > +	mvneta_rx_csum(pp, desc_status, skb);
> > > +
> > > +	for (i = 0; i < num_frags; i++) {
> > > +		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> > > +				frags[i].bv_page, frags[i].bv_offset,
> > > +				skb_frag_size(&frags[i]), PAGE_SIZE);
> > > +		page_pool_release_page(rxq->page_pool, frags[i].bv_page);
> > > +	}
> > > +
> > > +	return skb;
> > > +}  
> > 
> > Here as well - is the plan to turn more of this function into common
> > code later on? Looks like most of this is not really driver specific.  
> 
> I agree. What about adding it when other drivers will add multi-buff support?
> (here we have even page_pool dependency)

I guess that's okay on the condition that you're going to be the one
adding the support to the next driver, or at least review it very
closely to make sure it's done.

In general vendors prove rather resistant to factoring code out, 
the snowflakes they feel they are.