On Tue, Mar 22, 2022 at 11:36:55PM IST, Martin KaFai Lau wrote:
> On Sun, Mar 20, 2022 at 09:25:00PM +0530, Kumar Kartikeya Dwivedi wrote:
> > @@ -820,9 +904,31 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
> > return -EOPNOTSUPP;
> > }
> >
> > - if (map->ops->map_check_btf)
> > + map->kptr_off_tab = btf_find_kptr(btf, value_type);
> > + if (map_value_has_kptr(map)) {
> > + if (!bpf_capable())
> > + return -EPERM;
> Not sure if this has been brought up.
>
> No need to bpf_map_free_kptr_off_tab() in the case?
>
Good catch, it should indeed be freed.
For the case of map_check_btf in caller, I'm relying on map_free callback to
handle the freeing.
> > + if (map->map_flags & BPF_F_RDONLY_PROG) {
> > + ret = -EACCES;
> > + goto free_map_tab;
> > + }
> > + if (map->map_type != BPF_MAP_TYPE_HASH &&
> > + map->map_type != BPF_MAP_TYPE_LRU_HASH &&
> > + map->map_type != BPF_MAP_TYPE_ARRAY) {
> > + ret = -EOPNOTSUPP;
> > + goto free_map_tab;
> > + }
> > + }
> If btf_find_kptr() returns err, it can be ignored and continue ?
>
> btw, it is quite unusual to store an err ptr in map.
> How about only stores NULL or a valid ptr in map->kptr_off_tab?
>
It allows us to report a clear error from process_kptr_func, similar to storing
error in place of spin_lock_off and timer_off, so for consistency I kept it
similar to those. But IS_ERR_OR_NULL still means no kptr_off_tab is present, in
places where it matters we don't distinguish between the two (e.g.
map_value_has_kptr also checks for both).
> > +
> > + if (map->ops->map_check_btf) {
> > ret = map->ops->map_check_btf(map, btf, key_type, value_type);
> > + if (ret < 0)
> > + goto free_map_tab;
> > + }
> >
> > + return ret;
> > +free_map_tab:
> > + bpf_map_free_kptr_off_tab(map);
> > return ret;
> > }
--
Kartikeya
