netdev - Re: [PATCH v2 net-next 3/9] net: mvneta: update mb bit before passing the xdp buffer to eBPF layer

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: <20200906090557.GB2785@lore-desk>
Date:   Sun, 6 Sep 2020 11:05:57 +0200
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Shay Agroskin <shayagr@...zon.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        lorenzo.bianconi@...hat.com, brouer@...hat.com,
        echaudro@...hat.com, sameehj@...zon.com, kuba@...nel.org,
        john.fastabend@...il.com, daniel@...earbox.net, ast@...nel.org
Subject: Re: [PATCH v2 net-next 3/9] net: mvneta: update mb bit before
 passing the xdp buffer to eBPF layer

[...]

> >  				rx_desc->buf_phys_addr,
> >  				len, dma_dir);
> > -	if (data_len > 0 && sinfo->nr_frags < MAX_SKB_FRAGS) {
> > -		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags];
> > +	if (data_len > 0 && *nfrags < MAX_SKB_FRAGS) {
> > +		skb_frag_t *frag = &sinfo->frags[*nfrags];
> >  		skb_frag_off_set(frag, pp->rx_offset_correction);
> >  		skb_frag_size_set(frag, data_len);
> >  		__skb_frag_set_page(frag, page);
> > -		sinfo->nr_frags++;
> > +		*nfrags = *nfrags + 1;
> 
> nit, why do you use nfrags variable instead of sinfo->nr_frags (as it was
> before this patch) ?
>                You doesn't seem to use the nfrags variable in the
> caller function and you update nr_frags as well.
>                If it's used in a different patch (haven't
> reviewed it all yet), maybe move it to that patch                ? (sorry in
> advance if I missed the logic here)

nfrags pointer is used to avoid the sinfo->nr_frags initialization in
mvneta_swbm_rx_frame() since skb_shared_info is not in the cache usually.
AFAIK the hw does not provide the info "this is the second descriptor" but
just "this the frist/last descriptor".

Regards,
Lorenzo

> 
> > +
> > +		if (rx_desc->status & MVNETA_RXD_LAST_DESC) {
> > +			sinfo->nr_frags = *nfrags;
> > +			xdp->mb = true;
> > +		}
> >  		rx_desc->buf_phys_addr = 0;
> >               ...

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)