lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 13 Oct 2020 00:13:39 -0700 From: Yonghong Song <yhs@...com> To: Vasily Averin <vvs@...tuozzo.com>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org> CC: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>, Andrii Nakryiko <andriin@...com> Subject: Re: [PATCH net] net: fix pos incrementment in ipv6_route_seq_next On 10/12/20 11:56 PM, Vasily Averin wrote: > Dear Yonghong Song, > thank you for reporting the problem. > As far as I understand the problem here is that pos is incremented in .start function. Yes. > > I do not,like an idea to avoid increment in ipv6_route_seq_next() > however ipv6_route_seq_start can provide fake argument instead. > > --- a/net/ipv6/ip6_fib.c > +++ b/net/ipv6/ip6_fib.c > @@ -2618,8 +2618,9 @@ static void *ipv6_route_seq_start(struct seq_file *seq, loff_t *pos) > iter->skip = *pos; > > if (iter->tbl) { > + loff_t p; > ipv6_route_seq_setup_walk(iter, net); > - return ipv6_route_seq_next(seq, NULL, pos); > + return ipv6_route_seq_next(seq, NULL, &p); > } else { > return NULL; > } This should work too. I am fine with the change. I will wait until tomorrow for additional comments, if any, about this fake point approach vs. my approach, before sending v2. > > In this case patch subject should be changed accordingly. > > Thank you, > Vasily Averin > > On 10/13/20 3:09 AM, Yonghong Song wrote: >> Commit 4fc427e05158 ("ipv6_route_seq_next should increase position index") >> tried to fix the issue where seq_file pos is not increased >> if a NULL element is returned with seq_ops->next(). See bug >> https://bugzilla.kernel.org/show_bug.cgi?id=206283 >> The commit effectively does: >> - increase pos for all seq_ops->start() >> - increase pos for all seq_ops->next() >> >> For ipv6_route, increasing pos for all seq_ops->next() is correct. >> But increasing pos for seq_ops->start() is not correct >> since pos is used to determine how many items to skip during >> seq_ops->start(): >> iter->skip = *pos; >> seq_ops->start() just fetches the *current* pos item. >> The item can be skipped only after seq_ops->show() which essentially >> is the beginning of seq_ops->next(). >> >> For example, I have 7 ipv6 route entries, >> root@...h-fb-vm1:~/net-next dd if=/proc/net/ipv6_route bs=4096 >> 00000000000000000000000000000000 40 00000000000000000000000000000000 00 00000000000000000000000000000000 00000400 00000001 00000000 00000001 eth0 >> fe800000000000000000000000000000 40 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000001 00000000 00000001 eth0 >> 00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200 lo >> 00000000000000000000000000000001 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000003 00000000 80200001 lo >> fe800000000000002050e3fffebd3be8 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000002 00000000 80200001 eth0 >> ff000000000000000000000000000000 08 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000004 00000000 00000001 eth0 >> 00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200 lo >> 0+1 records in >> 0+1 records out >> 1050 bytes (1.0 kB, 1.0 KiB) copied, 0.00707908 s, 148 kB/s >> root@...h-fb-vm1:~/net-next >> >> In the above, I specify buffer size 4096, so all records can be returned >> to user space with a single trip to the kernel. >> >> If I use buffer size 128, since each record size is 149, internally >> kernel seq_read() will read 149 into its internal buffer and return the data >> to user space in two read() syscalls. Then user read() syscall will trigger >> next seq_ops->start(). Since the current implementation increased pos even >> for seq_ops->start(), it will skip record #2, #4 and #6, assuming the first >> record is #1. >> >> root@...h-fb-vm1:~/net-next dd if=/proc/net/ipv6_route bs=128 >> 00000000000000000000000000000000 40 00000000000000000000000000000000 00 00000000000000000000000000000000 00000400 00000001 00000000 00000001 eth0 >> 00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200 lo >> fe800000000000002050e3fffebd3be8 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000002 00000000 80200001 eth0 >> 00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200 lo >> 4+1 records in >> 4+1 records out >> 600 bytes copied, 0.00127758 s, 470 kB/s >> >> To fix the problem, do not increase pos for seq_ops->start() and the >> above `dd` command with `bs=128` will show correct result. >> >> Fixes: 4fc427e05158 ("ipv6_route_seq_next should increase position index") >> Cc: Vasily Averin <vvs@...tuozzo.com> >> Cc: Andrii Nakryiko <andriin@...com> >> Cc: Alexei Starovoitov <ast@...nel.org> >> Signed-off-by: Yonghong Song <yhs@...com> >> --- >> net/ipv6/ip6_fib.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c >> index 141c0a4c569a..5aac5094bc41 100644 >> --- a/net/ipv6/ip6_fib.c >> +++ b/net/ipv6/ip6_fib.c >> @@ -2582,10 +2582,10 @@ static void *ipv6_route_seq_next(struct seq_file *seq, void *v, loff_t *pos) >> struct net *net = seq_file_net(seq); >> struct ipv6_route_iter *iter = seq->private; >> >> - ++(*pos); >> if (!v) >> goto iter_table; >> >> + ++(*pos); >> n = rcu_dereference_bh(((struct fib6_info *)v)->fib6_next); >> if (n) >> return n; >>
Powered by blists - more mailing lists