netdev - Re: [PATCHv14 bpf-next 3/6] xdp: add a new helper for dev map multicast support

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: <6005a5f08de7a_23982089d@john-XPS-13-9370.notmuch>
Date:   Mon, 18 Jan 2021 07:14:56 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Hangbin Liu <liuhangbin@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Jiri Benc <jbenc@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>, ast@...nel.org,
        Daniel Borkmann <daniel@...earbox.net>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        David Ahern <dsahern@...il.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCHv14 bpf-next 3/6] xdp: add a new helper for dev map
 multicast support

Toke Høiland-Jørgensen wrote:
> Hangbin Liu <liuhangbin@...il.com> writes:
> 
> > Hi John,
> >
> > Thanks for the reviewing.
> >
> > On Sun, Jan 17, 2021 at 04:10:40PM -0800, John Fastabend wrote:
> >> > + * 		The forwarding *map* could be either BPF_MAP_TYPE_DEVMAP or
> >> > + * 		BPF_MAP_TYPE_DEVMAP_HASH. But the *ex_map* must be
> >> > + * 		BPF_MAP_TYPE_DEVMAP_HASH to get better performance.
> >> 
> >> Would be good to add a note ex_map _must_ be keyed by ifindex for the
> >> helper to work. Its the obvious way to key a hashmap, but not required
> >> iirc.
> >
> > OK, I will.

[...]

> >> WRITE_ONCE(ri->ex_map)?
> >> 
> >> >  	WRITE_ONCE(ri->map, NULL);
> >> 
> >> So we needed write_once, read_once pairs for ri->map do we also need them in
> >> the ex_map case?
> >
> > Toke said this is no need for this read/write_once as there is already one.
> >
> > https://lore.kernel.org/bpf/87r1wd2bqu.fsf@toke.dk/
> 
> And then I corrected that after I figured out the real reason :)
> 
> https://lore.kernel.org/bpf/878si2h3sb.fsf@toke.dk/ - Quote:
> 
> > The READ_ONCE() is not needed because the ex_map field is only ever read
> > from or written to by the CPU owning the per-cpu pointer. Whereas the
> > 'map' field is manipulated by remote CPUs in bpf_clear_redirect_map().
> > So you need neither READ_ONCE() nor WRITE_ONCE() on ex_map, just like
> > there are none on tgt_index and tgt_value.
> 
> -Toke
> 

Hi Hangbin, please add a comment above that code block to remind us
why the READ_ONCE/WRITE_ONCE is not needed or add it in the commit
message so we don't lose it. It seems we've hashed it over already,
but I forgot after the holidays/break so presumably I'll forget next
time I read this code as well and commit-msg or comment will help.

Thanks,
John