From: Miklos Szeredi <miklos@szeredi.hu>
To: "Eric W. Biederman" <ebiederm@xmission.com>,
"Serge H. Hallyn" <serge.hallyn@ubuntu.com>,
Andy Lutomirski <luto@amacapital.net>,
Michael j Theall <mtheall@us.ibm.com>,
fuse-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v5 1/4] fuse: Add support for pid namespaces
Date: Wed, 12 Nov 2014 13:07:42 +0100 [thread overview]
Message-ID: <20141112120742.GF333@tucsk> (raw)
In-Reply-To: <20141111152429.GA7906@ubuntu-hedt>
On Tue, Nov 11, 2014 at 09:24:29AM -0600, Seth Forshee wrote:
> > What happens when the server does indeed change pid namespace after mounting?
> > Will just receive bogus pid values? Shouldn't it receive an error instead?
>
> Yeah, I suppose it does receive bogus pids and userid values. About all
> we could do to prevent this is make the /dev/fuse read/write paths
> return an error if the current namespace isn't the same as the one at
> mount time. But then requests could get stuck in the queue forever, so
> maybe we should also fail all requests in the queue when this happens.
> Unless you have a better idea?
In fuse_dev_do_read() just after dequeuing the request check if the namespaces
match, and if not, reject with EIO.
> > > @@ -2146,7 +2147,11 @@ static int convert_fuse_file_lock(const struct fuse_file_lock *ffl,
> > >
> > > fl->fl_start = ffl->start;
> > > fl->fl_end = ffl->end;
> > > - fl->fl_pid = ffl->pid;
> > > + rcu_read_lock();
> > > + fl->fl_pid = pid_vnr(find_pid_ns(ffl->pid, fc->pid_ns));
> > > + rcu_read_unlock();
> > > + if (ffl->pid != 0 && fl->fl_pid == 0)
> > > + return -EIO;
> >
> > This needs some comments: is this trying to translate the pid backwards?
> > Why is it not checking the return of find_pid_ns() then? The man page
> > documents l_pid value of -1 but not of 0, so why are we checking for
> > "ffl->pid != 0"? Or is the man page incomplete and in practice we get l_pid
> > == 0 values?
>
> I'll add comments. It's converting the pid in the fuse_file_lock into
> the current pid namespace. pid_vnr calls pid_nr_ns, which returns 0 if
> the pid can't be translated into the namespace, thus we return the
> error.
>
> pid_vnr's return values don't necessarily conform to the expectations of
> the fcntl syscall in all cases, and as far as I can tell it should never
> return <0. But if fcntl doesn't expect l_pid == 0 and pid_vnr could
> return that value then doesn't it makes sense for fuse to return an
> error in cases where this would happen?
Not necessarily. The conflicting lock might be held by a process whose pid is
not visible from the client's namespace. That doesn't mean that the GETLK
should fail, it just means that l_pid can't be filled in (same as in NFS when a
lock held on a different client). AFAICS, NFS fills l_pid with zero in that
case. Makes sense, not sure why the man page doesn't document that.
> > > @@ -2170,7 +2175,9 @@ static void fuse_lk_fill(struct fuse_req *req, struct file *file,
> > > arg->lk.start = fl->fl_start;
> > > arg->lk.end = fl->fl_end;
> > > arg->lk.type = fl->fl_type;
> > > - arg->lk.pid = pid;
> > > + arg->lk.pid = pid_nr_ns(pid, fc->pid_ns);
> > > + if (pid && arg->lk.pid == 0)
> > > + return -EOVERFLOW;
> >
> > Could have done the conversion immediately after getting 'pid' with task_tgid(),
> > then the changes would have been smaller and more localized.
>
> The changes would be very marginally smaller since currently only one
> caller of fuse_lk_fill which passes a non-zero pid. If additional
> callers were ever added with non-zero pids then there would be
> duplication of this code. But I'll do it whichever way you prefer, just
> let me know.
I prefer the simpler (even if only marginally) one.
Thanks,
Miklos
next prev parent reply other threads:[~2014-11-12 12:07 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-22 21:24 [PATCH v5 0/4] fuse: Add support for mounts from pid/user namespaces Seth Forshee
2014-10-22 21:24 ` [PATCH v5 1/4] fuse: Add support for pid namespaces Seth Forshee
2014-11-11 13:27 ` Miklos Szeredi
2014-11-11 15:24 ` Seth Forshee
2014-11-11 15:39 ` Andy Lutomirski
2014-11-11 16:26 ` Seth Forshee
2014-11-12 12:07 ` Miklos Szeredi [this message]
2014-11-12 14:33 ` Seth Forshee
2014-10-22 21:24 ` [PATCH v5 2/4] fuse: Support fuse filesystems outside of init_user_ns Seth Forshee
2014-10-22 21:47 ` Andy Lutomirski
2014-11-11 14:04 ` Miklos Szeredi
2014-11-11 15:27 ` Seth Forshee
2014-11-11 15:37 ` Eric W. Biederman
2014-11-12 13:09 ` Miklos Szeredi
2014-11-12 16:22 ` Seth Forshee
2014-11-18 15:21 ` Seth Forshee
2014-11-18 17:09 ` Andy Lutomirski
2014-11-18 17:13 ` Seth Forshee
2014-11-18 17:19 ` Andy Lutomirski
2014-11-19 8:50 ` Miklos Szeredi
2014-11-19 10:38 ` Miklos Szeredi
2014-11-19 14:09 ` Serge E. Hallyn
2014-11-21 16:44 ` Seth Forshee
2014-11-21 17:19 ` Andy Lutomirski
2014-11-21 18:14 ` Eric W. Biederman
2014-11-21 18:25 ` Andy Lutomirski
2014-11-21 18:27 ` Seth Forshee
2014-11-21 18:38 ` Andy Lutomirski
2014-10-22 21:24 ` [PATCH v5 3/4] fuse: Restrict allow_other to the superblock's namespace or a descendant Seth Forshee
2014-10-22 21:48 ` Andy Lutomirski
2014-11-11 15:27 ` Miklos Szeredi
2014-11-11 15:37 ` Seth Forshee
2014-10-22 21:24 ` [PATCH v5 4/4] fuse: Allow user namespace mounts Seth Forshee
2014-10-22 21:51 ` Andy Lutomirski
2014-10-23 0:22 ` Seth Forshee
2014-10-23 2:19 ` Andy Lutomirski
2014-11-03 17:15 ` [PATCH v5 0/4] fuse: Add support for mounts from pid/user namespaces Seth Forshee
2014-11-03 17:17 ` Andy Lutomirski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20141112120742.GF333@tucsk \
--to=miklos@szeredi.hu \
--cc=ebiederm@xmission.com \
--cc=fuse-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mtheall@us.ibm.com \
--cc=serge.hallyn@ubuntu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).