On Wed, Jul 23, 2014 at 1:25 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Wed, Jul 23, 2014 at 12:49 PM, Alexei Starovoitov <ast@xxxxxxxxxxxx> wrote:
>> On Wed, Jul 23, 2014 at 11:25 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>>> +
>>>> + /* lookup key in a given map referenced by map_id
>>>> + * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
>>>
>>> This needs map_id documentation updates too?
>>
>> yes. will grep for it just to make sure.
>>
>>>> +static int get_map_id(struct fd f)
>>>> +{
>>>> + struct bpf_map *map;
>>>> +
>>>> + if (!f.file)
>>>> + return -EBADF;
>>>> +
>>>> + if (f.file->f_op != &bpf_map_fops) {
>>>> + fdput(f);
>>>
>>> It feels weird to me to do the fdput inside this function. Instead,
>>> should map_lookup_elem get a "err_put" label, instead?
>>
>> I don't think it will work, since I'm not sure that fd.flags will be zero
>> when fd.file == NULL. It looks so by analyzing return code path
>> in fs/file.c, but I wasn't sure that I followed all code paths,
>> so I just picked this style from fs/timerfd.c assuming it was
>> done this away on purpose and there can be the case where
>> fd.file == null and fd.flags !=0. In such case we cannot call fdput().
>
> Yeah, hm, looking around, this does seem to be the case. I guess the
> thought is that when get_map_id fails, struct fd has been handled.
correct.
> Maybe add a comment above that function as a reminder?
yes. will do.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
