Christoph Hellwig <hch@xxxxxx> 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@xxxxxx>
> ---
> 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)
