| Message ID | d6e2df39-919e-8d37-0668-5c4bbf19f278@virtuozzo.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: seq_file .next functions should increase position index | expand |
On 1/24/20 7:17 AM, Vasily Averin wrote: > if seq_file .next fuction does not change position index, > read after some lseek can generate unexpected output. > > https://bugzilla.kernel.org/show_bug.cgi?id=206283 > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > kernel/bpf/inode.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c > index ecf42be..9008a20 100644 > --- a/kernel/bpf/inode.c > +++ b/kernel/bpf/inode.c > @@ -196,6 +196,7 @@ static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) > void *key = map_iter(m)->key; > void *prev_key; > > + (*pos)++; > if (map_iter(m)->done) > return NULL; > > Hm, how did you test this change? Please elaborate, since in map_seq_next() we do increment position index: static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) { struct bpf_map *map = seq_file_to_map(m); void *key = map_iter(m)->key; void *prev_key; if (map_iter(m)->done) return NULL; if (unlikely(v == SEQ_START_TOKEN)) prev_key = NULL; else prev_key = key; if (map->ops->map_get_next_key(map, prev_key, key)) { map_iter(m)->done = true; return NULL; } ++(*pos); <------------ here return key; } With your change we'd increment twice. What is missing here? Thanks, Daniel
On 1/24/20 1:03 PM, Daniel Borkmann wrote: > On 1/24/20 7:17 AM, Vasily Averin wrote: >> if seq_file .next fuction does not change position index, >> read after some lseek can generate unexpected output. >> >> https://bugzilla.kernel.org/show_bug.cgi?id=206283 >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >> --- >> kernel/bpf/inode.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c >> index ecf42be..9008a20 100644 >> --- a/kernel/bpf/inode.c >> +++ b/kernel/bpf/inode.c >> @@ -196,6 +196,7 @@ static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) >> void *key = map_iter(m)->key; >> void *prev_key; >> + (*pos)++; >> if (map_iter(m)->done) >> return NULL; >> > > Hm, how did you test this change? Please elaborate, since in map_seq_next() > we do increment position index: Position index should be updated even if .next returns NULL. Sorry, I've not tested this case. $ dd if=AFFECTED_FILE bs=1000 skip=1 With any huge bs it will generate last line. If you'll specify bs in middle of last line -- dd will output rest of list line and then whole last line once again. > static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) > { > struct bpf_map *map = seq_file_to_map(m); > void *key = map_iter(m)->key; > void *prev_key; > > if (map_iter(m)->done) > return NULL; > > if (unlikely(v == SEQ_START_TOKEN)) > prev_key = NULL; > else > prev_key = key; > > if (map->ops->map_get_next_key(map, prev_key, key)) { > map_iter(m)->done = true; > return NULL; > } > > ++(*pos); <------------ here You are right, I've missed it, it should be removed. > return key; > } > > With your change we'd increment twice. What is missing here?
On 1/24/20 12:03 PM, Vasily Averin wrote: > On 1/24/20 1:03 PM, Daniel Borkmann wrote: >> On 1/24/20 7:17 AM, Vasily Averin wrote: >>> if seq_file .next fuction does not change position index, >>> read after some lseek can generate unexpected output. >>> >>> https://bugzilla.kernel.org/show_bug.cgi?id=206283 >>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >>> --- >>> kernel/bpf/inode.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c >>> index ecf42be..9008a20 100644 >>> --- a/kernel/bpf/inode.c >>> +++ b/kernel/bpf/inode.c >>> @@ -196,6 +196,7 @@ static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) >>> void *key = map_iter(m)->key; >>> void *prev_key; >>> + (*pos)++; >>> if (map_iter(m)->done) >>> return NULL; >>> >> >> Hm, how did you test this change? Please elaborate, since in map_seq_next() >> we do increment position index: > > Position index should be updated even if .next returns NULL. > > Sorry, I've not tested this case. > > $ dd if=AFFECTED_FILE bs=1000 skip=1 > > With any huge bs it will generate last line. > If you'll specify bs in middle of last line -- > dd will output rest of list line and then whole last line once again. > >> static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) >> { >> struct bpf_map *map = seq_file_to_map(m); >> void *key = map_iter(m)->key; >> void *prev_key; >> >> if (map_iter(m)->done) >> return NULL; >> >> if (unlikely(v == SEQ_START_TOKEN)) >> prev_key = NULL; >> else >> prev_key = key; >> >> if (map->ops->map_get_next_key(map, prev_key, key)) { >> map_iter(m)->done = true; >> return NULL; >> } >> >> ++(*pos); <------------ here > > You are right, I've missed it, it should be removed. Ok, please respin v2 then. Thanks, Daniel
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c index ecf42be..9008a20 100644 --- a/kernel/bpf/inode.c +++ b/kernel/bpf/inode.c @@ -196,6 +196,7 @@ static void *map_seq_next(struct seq_file *m, void *v, loff_t *pos) void *key = map_iter(m)->key; void *prev_key; + (*pos)++; if (map_iter(m)->done) return NULL;
if seq_file .next fuction does not change position index, read after some lseek can generate unexpected output. https://bugzilla.kernel.org/show_bug.cgi?id=206283 Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- kernel/bpf/inode.c | 1 + 1 file changed, 1 insertion(+)