On 03/12/2019 12:06 AM, Alexei Starovoitov wrote: > On Mon, Mar 11, 2019 at 10:51:18PM +0100, Daniel Borkmann wrote: >> This work adds two new map creation flags BPF_F_RDONLY_PROG >> and BPF_F_WRONLY_PROG in order to allow for read-only or >> write-only BPF maps from a BPF program side. >> >> Today we have BPF_F_RDONLY and BPF_F_WRONLY, but this only >> applies to system call side, meaning the BPF program has full >> read/write access to the map as usual while bpf(2) calls with >> map fd can either only read or write into the map depending >> on the flags. BPF_F_RDONLY_PROG and BPF_F_WRONLY_PROG allows >> for the exact opposite such that verifier is going to reject >> program loads if write into a read-only map or a read into a >> write-only map is detected. For read-only map case also some >> helpers are forbidden for programs that would alter the map >> state such as map deletion, update, etc. >> >> We've enabled this generic map extension to various non-special >> maps holding normal user data: array, hash, lru, lpm, local >> storage, queue and stack. Further map types could be followed >> up in future depending on use-case. Main use case here is to >> forbid writes into .rodata map values from verifier side. > > I think WRONLY | WRONLY_PROG should be invalid combination? > Since these attributes are set at creation time and cannot be changed, > nothing can ever read from it, so why write into it? > Similarly RDONLY | RDONLY_PROG is invalid too? Yeah, I can add this. Note that 'these attributes are set at creation time and cannot be changed' does not fully hold for WRONLY/RDONLY, e.g. you can create a map as RDONLY, but later on retrieve fd by id or from bpf fs but without RDONLY, so this fd will be able to write into it from syscall side (we're checking struct file flags, not the attributes' map flags at runtime). Given that I thought it made more sense to keep these two logically separated since one is only revelant for lifetime of fd and the other one for map. (I also think keeping RDONLY/WRONLY stored in map_flags attr and exposing it is pretty confusing since it doesn't say anything, this should probably be masked out as a fix, thoughts?) > Also looking at the next patch and 'lock' command... > May be it would be cleaner to do add WRONCE (from syscall) flag? > Then for .rodata the attrs will be RDONLY_PROG | WRONCE > and no 'lock' necessary. > WRONCE_PROG probably doesn't make sense. Agree, from prog side not and would probably also just make fast-path slower. Hm, for the WRONCE at creation flags, we'd need some form of locking on syscall side to avoid racing I'd think. The lock cmd, simply just does the write_once() and rcu sync to wait for everything to complete, so this is pretty trivial w/o slowing down anything on syscall side. I can take a look, but feels worse to me right now. The other thing is, given RDONLY/WRONLY semantics are _only_ for fd and _not_ map, mixing these might be confusing as well, since WRONCE would then be a map property whereas RDONLY/WRONLY not. > Storing dangling task_struct pointer in the next patch doesn't look great. Hm, it's actually not dangling, on close we exchange it with NULL (which current can never be) such that either root or only original map creator may lock it as read-only. > The whole 'lock' concept feels useful, but in the context of implementing > .rodata it feels that WRONCE would be a better fit, > since libbpf won't be able to make a mistake and forget to 'lock'. > 'lock' syscall cmd can be confused with BPF_F_LOCK too. That's a naming detail, but sure it can be changed. :) Thanks, Daniel
