| Message ID | 1401919352-32263-1-git-send-email-sasha.levin@oracle.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 6/4/14, 4:02 PM, Sasha Levin wrote: > This reverts commit 30f38d2fdd79f13fc929489f7e6e517b4a4bfe63. > > fib_triestat is surrounded by a big lie: while it claims that it's a > seq_file (fib_triestat_seq_open, fib_triestat_seq_show), it isn't: > > static const struct file_operations fib_triestat_fops = { > .owner = THIS_MODULE, > .open = fib_triestat_seq_open, > .read = seq_read, > .llseek = seq_lseek, > .release = single_release_net, > }; > > Yes, fib_triestat is just a regular file. > > A small detail (assuming CONFIG_NET_NS=y) is that while for seq_files > you could do seq_file_net() to get the net ptr, doing so for a regular > file would be wrong and would dereference an invalid pointer. > > The fib_triestat lie claimed a victim, and trying to show the file would > be bad for the kernel. This patch just reverts the issue and fixes > fib_triestat, which still needs a rewrite to either be a seq_file or > stop claiming it is. It worked fine for me. fib_triestat_seq_open() is a wrapper to single_open_net() which sets the namespace as the private element (returned by get_proc_net). If CONFIG_NETNS is not set: - single_open_net essentially sets file->private to init_net which is also what is returned by seq_file_net If CONFIG_NETNS is set, all is fine. What am I missing? David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2014 02:02 AM, Sasha Levin wrote: > This reverts commit 30f38d2fdd79f13fc929489f7e6e517b4a4bfe63. > fib_triestat is surrounded by a big lie: while it claims that it's a > seq_file (fib_triestat_seq_open, fib_triestat_seq_show), it isn't: > static const struct file_operations fib_triestat_fops = { > .owner = THIS_MODULE, > .open = fib_triestat_seq_open, > .read = seq_read, > .llseek = seq_lseek, > .release = single_release_net, > }; > Yes, fib_triestat is just a regular file. > A small detail (assuming CONFIG_NET_NS=y) is that while for seq_files > you could do seq_file_net() to get the net ptr, doing so for a regular > file would be wrong and would dereference an invalid pointer. > > The fib_triestat lie claimed a victim, and trying to show the file would > be bad for the kernel. This patch just reverts the issue and fixes > fib_triestat, which still needs a rewrite to either be a seq_file or > stop claiming it is. > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > net/ipv4/fib_trie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c > index 243c7f4..5afeb5a 100644 > --- a/net/ipv4/fib_trie.c > +++ b/net/ipv4/fib_trie.c > @@ -2166,7 +2166,7 @@ static void fib_table_print(struct seq_file *seq, struct fib_table *tb) > > static int fib_triestat_seq_show(struct seq_file *seq, void *v) > { > - struct net *net = seq_file_net(seq); > + struct net *net = (struct net *)seq->private; 'void *' doesn't need any explicit casting to another pointer type, the cast it automatic. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sasha Levin <sasha.levin@oracle.com> Date: Wed, 4 Jun 2014 18:02:32 -0400 > This reverts commit 30f38d2fdd79f13fc929489f7e6e517b4a4bfe63. > > fib_triestat is surrounded by a big lie: while it claims that it's a > seq_file (fib_triestat_seq_open, fib_triestat_seq_show), it isn't: > > static const struct file_operations fib_triestat_fops = { > .owner = THIS_MODULE, > .open = fib_triestat_seq_open, > .read = seq_read, > .llseek = seq_lseek, > .release = single_release_net, > }; > > Yes, fib_triestat is just a regular file. > > A small detail (assuming CONFIG_NET_NS=y) is that while for seq_files > you could do seq_file_net() to get the net ptr, doing so for a regular > file would be wrong and would dereference an invalid pointer. > > The fib_triestat lie claimed a victim, and trying to show the file would > be bad for the kernel. This patch just reverts the issue and fixes > fib_triestat, which still needs a rewrite to either be a seq_file or > stop claiming it is. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> Thanks a lot for catching this, applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/4/14, 4:11 PM, David Ahern wrote: > On 6/4/14, 4:02 PM, Sasha Levin wrote: >> This reverts commit 30f38d2fdd79f13fc929489f7e6e517b4a4bfe63. >> >> fib_triestat is surrounded by a big lie: while it claims that it's a >> seq_file (fib_triestat_seq_open, fib_triestat_seq_show), it isn't: >> >> static const struct file_operations fib_triestat_fops = { >> .owner = THIS_MODULE, >> .open = fib_triestat_seq_open, >> .read = seq_read, >> .llseek = seq_lseek, >> .release = single_release_net, >> }; >> >> Yes, fib_triestat is just a regular file. If you compare fib_trie to fib_triestat the only difference is the use of single_open_net and single_release_net by fib_triestat. single_open_net is a wrapper to single_open passing struct net as the private data. single_open is for seq_files: int single_open(struct file *file, int (*show)(struct seq_file *, void *), void *data) { struct seq_operations *op = kmalloc(sizeof(*op), GFP_KERNEL); int res = -ENOMEM; if (op) { op->start = single_start; op->next = single_next; op->stop = single_stop; op->show = show; res = seq_open(file, op); if (!res) ((struct seq_file *)file->private_data)->private = data; Where is the problem using seq_file_net in fib_triestat_seq_show: static int fib_triestat_seq_show(struct seq_file *seq, void *v) { struct net *net = (struct net *)seq->private; where static inline struct net *seq_file_net(struct seq_file *seq) { #ifdef CONFIG_NET_NS return ((struct seq_net_private *)seq->private)->net; #else return &init_net; #endif } David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 243c7f4..5afeb5a 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -2166,7 +2166,7 @@ static void fib_table_print(struct seq_file *seq, struct fib_table *tb) static int fib_triestat_seq_show(struct seq_file *seq, void *v) { - struct net *net = seq_file_net(seq); + struct net *net = (struct net *)seq->private; unsigned int h; seq_printf(seq,
This reverts commit 30f38d2fdd79f13fc929489f7e6e517b4a4bfe63. fib_triestat is surrounded by a big lie: while it claims that it's a seq_file (fib_triestat_seq_open, fib_triestat_seq_show), it isn't: static const struct file_operations fib_triestat_fops = { .owner = THIS_MODULE, .open = fib_triestat_seq_open, .read = seq_read, .llseek = seq_lseek, .release = single_release_net, }; Yes, fib_triestat is just a regular file. A small detail (assuming CONFIG_NET_NS=y) is that while for seq_files you could do seq_file_net() to get the net ptr, doing so for a regular file would be wrong and would dereference an invalid pointer. The fib_triestat lie claimed a victim, and trying to show the file would be bad for the kernel. This patch just reverts the issue and fixes fib_triestat, which still needs a rewrite to either be a seq_file or stop claiming it is. Signed-off-by: Sasha Levin <sasha.levin@oracle.com> --- net/ipv4/fib_trie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)