On Mon, Apr 8, 2019 at 3:54 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> This patch adds a new BPF_MAP_FREEZE command which allows to
> "freeze" the map globally as read-only / immutable from syscall
> side.
>
> Map permission handling has been refactored into map_get_sys_perms()
> and drops FMODE_CAN_WRITE in case of locked map. Main use case is
> to allow for setting up .rodata sections from the BPF ELF which
> are loaded into the kernel, meaning BPF loader first allocates
> map, sets up map value by copying .rodata section into it and once
> complete, it calls BPF_MAP_FREEZE on the map fd to prevent further
> modifications.
>
> Right now BPF_MAP_FREEZE only takes map fd as argument while remaining
> bpf_attr members are required to be zero. I didn't add write-only
> locking here as counterpart since I don't have a concrete use-case
> for it on my side, and I think it makes probably more sense to wait
> once there is actually one. In that case bpf_attr can be extended
> as usual with a flag field and/or others where flag 0 means that
> we lock the map read-only hence this doesn't prevent to add further
> extensions to BPF_MAP_FREEZE upon need.
>
> A map creation flag like BPF_F_WRONCE was not considered for couple
> of reasons: i) in case of a generic implementation, a map can consist
> of more than just one element, thus there could be multiple map
> updates needed to set the map into a state where it can then be
> made immutable, ii) WRONCE indicates exact one-time write before
> it is then set immutable. A generic implementation would set a bit
> atomically on map update entry (if unset), indicating that every
> subsequent update from then onwards will need to bail out there.
> However, map updates can fail, so upon failure that flag would need
> to be unset again and the update attempt would need to be repeated
> for it to be eventually made immutable. While this can be made
> race-free, this approach feels less clean and in combination with
> reason i), it's not generic enough. A dedicated BPF_MAP_FREEZE
> command directly sets the flag, allows all pending operations to
> finish and caller has the guarantee that map is immutable from
> syscall side upon successful return, which is also more intuitive
> from an API point of view. A command name such as BPF_MAP_LOCK has
> been avoided as it's too close with BPF map spin locks (which already
> has BPF_F_LOCK flag). BPF_MAP_FREEZE is so far only enabled for
> privileged users.
[...]
> @@ -857,8 +870,7 @@ static int map_update_elem(union bpf_attr *attr)
> map = __bpf_map_get(f);
> if (IS_ERR(map))
> return PTR_ERR(map);
> -
> - if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
> + if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) {
> err = -EPERM;
> goto err_put;
> }
[...]
> +static int map_freeze(const union bpf_attr *attr)
> +{
> + int err = 0, ufd = attr->map_fd;
> + struct bpf_map *map;
> + struct fd f;
> +
> + if (CHECK_ATTR(BPF_MAP_FREEZE))
> + return -EINVAL;
> +
> + f = fdget(ufd);
> + map = __bpf_map_get(f);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> + if (READ_ONCE(map->frozen)) {
> + err = -EBUSY;
> + goto err_put;
> + }
> + if (!capable(CAP_SYS_ADMIN)) {
> + err = -EPERM;
> + goto err_put;
> + }
> +
> + WRITE_ONCE(map->frozen, true);
> + synchronize_rcu();
What is this synchronize_rcu() doing? It seems like your intent might
be to ensure that all pending writes from the syscall side have
finished by the time this returns, but functions like
map_update_elem() aren't in an RCU read-side critical section when
they check for ->frozen, so that doesn't work, right?
> +err_put:
> + fdput(f);
> + return err;
> +}
