| Message ID | 20200722195156.4029817-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf: fix pos computation for bpf_iter seq_ops->start() | expand |
On Wed, Jul 22, 2020 at 12:52 PM Yonghong Song <yhs@fb.com> wrote: > > Currently, the pos pointer in bpf iterator map/task/task_file > seq_ops->start() is always incremented. > This is incorrect. It should be increased only if > *pos is 0 (for SEQ_START_TOKEN) since these start() > function actually returns the first real object. > If *pos is not 0, it merely found the object > based on the state in seq->private, and not really > advancing the *pos. This patch fixed this issue > by only incrementing *pos if it is 0. > > Note that the old *pos calculation, although not > correct, does not affect correctness of bpf_iter > as bpf_iter seq_file->read() does not support llseek. > > This patch also renamed "mid" in bpf_map iterator > seq_file private data to "map_id" for better clarity. > > Fixes: 6086d29def80 ("bpf: Add bpf_map iterator") > Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets") > Signed-off-by: Yonghong Song <yhs@fb.com> Applied. Thanks
On Wed, Jul 22, 2020 at 12:52 PM Yonghong Song <yhs@fb.com> wrote: > > Alexei, I also made the change of "mid"->"map_id" > and simplified the logic in map_iter seq_file->next() Thanks! > the same as your patch in > https://lore.kernel.org/bpf/20200717044031.56412-2-alexei.starovoitov@gmail.com/T I've rebased that one on top of your patch and applied to bpf-next. Thanks
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c index 8a7af11b411f..5926c76d854e 100644 --- a/kernel/bpf/map_iter.c +++ b/kernel/bpf/map_iter.c @@ -7,7 +7,7 @@ #include <linux/btf_ids.h> struct bpf_iter_seq_map_info { - u32 mid; + u32 map_id; }; static void *bpf_map_seq_start(struct seq_file *seq, loff_t *pos) @@ -15,27 +15,23 @@ static void *bpf_map_seq_start(struct seq_file *seq, loff_t *pos) struct bpf_iter_seq_map_info *info = seq->private; struct bpf_map *map; - map = bpf_map_get_curr_or_next(&info->mid); + map = bpf_map_get_curr_or_next(&info->map_id); if (!map) return NULL; - ++*pos; + if (*pos == 0) + ++*pos; return map; } static void *bpf_map_seq_next(struct seq_file *seq, void *v, loff_t *pos) { struct bpf_iter_seq_map_info *info = seq->private; - struct bpf_map *map; ++*pos; - ++info->mid; + ++info->map_id; bpf_map_put((struct bpf_map *)v); - map = bpf_map_get_curr_or_next(&info->mid); - if (!map) - return NULL; - - return map; + return bpf_map_get_curr_or_next(&info->map_id); } struct bpf_iter__bpf_map { diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c index 2feecf095609..1039e52ebd8b 100644 --- a/kernel/bpf/task_iter.c +++ b/kernel/bpf/task_iter.c @@ -51,7 +51,8 @@ static void *task_seq_start(struct seq_file *seq, loff_t *pos) if (!task) return NULL; - ++*pos; + if (*pos == 0) + ++*pos; return task; } @@ -210,7 +211,8 @@ static void *task_file_seq_start(struct seq_file *seq, loff_t *pos) return NULL; } - ++*pos; + if (*pos == 0) + ++*pos; info->task = task; info->files = files;
Currently, the pos pointer in bpf iterator map/task/task_file seq_ops->start() is always incremented. This is incorrect. It should be increased only if *pos is 0 (for SEQ_START_TOKEN) since these start() function actually returns the first real object. If *pos is not 0, it merely found the object based on the state in seq->private, and not really advancing the *pos. This patch fixed this issue by only incrementing *pos if it is 0. Note that the old *pos calculation, although not correct, does not affect correctness of bpf_iter as bpf_iter seq_file->read() does not support llseek. This patch also renamed "mid" in bpf_map iterator seq_file private data to "map_id" for better clarity. Fixes: 6086d29def80 ("bpf: Add bpf_map iterator") Fixes: eaaacd23910f ("bpf: Add task and task/file iterator targets") Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/map_iter.c | 16 ++++++---------- kernel/bpf/task_iter.c | 6 ++++-- 2 files changed, 10 insertions(+), 12 deletions(-) I targeted this to bpf-next as I think this patch does not need to backport to bpf tree as it does not impact correctness. Alexei, I also made the change of "mid"->"map_id" and simplified the logic in map_iter seq_file->next() the same as your patch in https://lore.kernel.org/bpf/20200717044031.56412-2-alexei.starovoitov@gmail.com/T the goal is to reduce conflicts as we touch the same file and close lines.