Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753086AbaKLOdY (ORCPT ); Wed, 12 Nov 2014 09:33:24 -0500 Received: from mail-ob0-f171.google.com ([209.85.214.171]:61207 "EHLO mail-ob0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753033AbaKLOdU (ORCPT ); Wed, 12 Nov 2014 09:33:20 -0500 Date: Wed, 12 Nov 2014 08:33:14 -0600 From: Seth Forshee To: Miklos Szeredi Cc: "Eric W. Biederman" , "Serge H. Hallyn" , Andy Lutomirski , Michael j Theall , fuse-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, seth.forshee@canonical.com Subject: Re: [PATCH v5 1/4] fuse: Add support for pid namespaces Message-ID: <20141112143314.GA31775@ubuntu-hedt> Mail-Followup-To: Miklos Szeredi , "Eric W. Biederman" , "Serge H. Hallyn" , Andy Lutomirski , Michael j Theall , fuse-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org References: <1414013060-137148-1-git-send-email-seth.forshee@canonical.com> <1414013060-137148-2-git-send-email-seth.forshee@canonical.com> <20141111132734.GC333@tucsk> <20141111152429.GA7906@ubuntu-hedt> <20141112120742.GF333@tucsk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141112120742.GF333@tucsk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 12, 2014 at 01:07:42PM +0100, Miklos Szeredi wrote: > 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. Okay, I'll do that. > > > > @@ -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. Seems reasonable to follow the precedence set by NFS then. > > > > > @@ -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. I'll change it then. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/