Messages in this thread | | | From | (Eric W. Biederman) | Date | Sat, 05 May 2018 07:37:33 -0500 | Subject | Re: [PATCH 11/40] ipv6/flowlabel: simplify pid namespace lookup |
| |
Christoph Hellwig <hch@lst.de> writes:
> The shole seq_file sequence already operates under a single RCU lock pair, > so move the pid namespace lookup into it, and stop grabbing a reference > and remove all kinds of boilerplate code.
This is wrong.
Move task_active_pid_ns(current) from open to seq_start actually means that the results if you pass this proc file between callers the results will change. So this breaks file descriptor passing.
Open is a bad place to access current. In the middle of read/write is broken.
In this particular instance looking up the pid namespace with task_active_pid_ns was a personal brain fart. What the code should be doing (with an appropriate helper) is:
struct pid_namespace *pid_ns = inode->i_sb->s_fs_info;
Because each mount of proc is bound to a pid namespace. Looking up the pid namespace from the super_block is a much better way to go.
Eric
> Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > net/ipv6/ip6_flowlabel.c | 28 +++++----------------------- > 1 file changed, 5 insertions(+), 23 deletions(-) > > diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c > index c05c4e82a7ca..a9f221d45ef9 100644 > --- a/net/ipv6/ip6_flowlabel.c > +++ b/net/ipv6/ip6_flowlabel.c > @@ -754,7 +754,10 @@ static struct ip6_flowlabel *ip6fl_get_idx(struct seq_file *seq, loff_t pos) > static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos) > __acquires(RCU) > { > + struct ip6fl_iter_state *state = ip6fl_seq_private(seq); > + > rcu_read_lock_bh(); > + state->pid_ns = task_active_pid_ns(current); > return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN; > } > > @@ -810,36 +813,15 @@ static const struct seq_operations ip6fl_seq_ops = { > > static int ip6fl_seq_open(struct inode *inode, struct file *file) > { > - struct seq_file *seq; > - struct ip6fl_iter_state *state; > - int err; > - > - err = seq_open_net(inode, file, &ip6fl_seq_ops, > + return seq_open_net(inode, file, &ip6fl_seq_ops, > sizeof(struct ip6fl_iter_state)); > - > - if (!err) { > - seq = file->private_data; > - state = ip6fl_seq_private(seq); > - rcu_read_lock(); > - state->pid_ns = get_pid_ns(task_active_pid_ns(current)); > - rcu_read_unlock(); > - } > - return err; > -} > - > -static int ip6fl_seq_release(struct inode *inode, struct file *file) > -{ > - struct seq_file *seq = file->private_data; > - struct ip6fl_iter_state *state = ip6fl_seq_private(seq); > - put_pid_ns(state->pid_ns); > - return seq_release_net(inode, file); > } > > static const struct file_operations ip6fl_seq_fops = { > .open = ip6fl_seq_open, > .read = seq_read, > .llseek = seq_lseek, > - .release = ip6fl_seq_release, > + .release = seq_release_net, > }; > > static int __net_init ip6_flowlabel_proc_init(struct net *net)
| |