Linus,
Please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus
to receive the following fuse fixes.
Thanks,
Miklos
----
Miklos Szeredi (1):
fuse: fix oops in revalidate when called with NULL nameidata
---
fs/fuse/dir.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
Hmm.
Do we really ever have a NULL 'nd' these days? Can you send me the
backtrace for whatever oops that was reported?
The reason I ask is because at least NFS also does just
if (nd->flags & LOOKUP_RCU)
return -ECHILD;
in its nfs_lookup/open_revalidate() functions. As does cifs, ncpfs, p9
and coda from a quick grep.
Linus
On Thu, May 12, 2011 at 8:04 AM, Miklos Szeredi <[email protected]> wrote:
>
> Please pull from
>
> ?git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus
>
> Miklos Szeredi (1):
> ? ? ?fuse: fix oops in revalidate when called with NULL nameidata
>
> ---
> ?fs/fuse/dir.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
Ooh.
It's "lookup_one_len()", isn't it?
So any filesystem that uses that helper will need to be protected from
a NULL 'nd'.
And then you have nfsd, ecryptfs and cachefiles that will do it on
_other_ filesystems.
Gaah. Ugly. So either we really should fix the filesystems that don't
have protection from a NULL nd, or we should fix lookup_one_len() (are
there perhaps other cases I missed?)
Al? Christoph? Comments?
Linus
On Thu, May 12, 2011 at 8:12 AM, Linus Torvalds
<[email protected]> wrote:
> Hmm.
>
> Do we really ever have a NULL 'nd' these days? Can you send me the
> backtrace for whatever oops that was reported?
>
> The reason I ask is because at least NFS also does just
>
> ? ? ? ?if (nd->flags & LOOKUP_RCU)
> ? ? ? ? ? ? ? ?return -ECHILD;
>
> in its nfs_lookup/open_revalidate() functions. As does cifs, ncpfs, p9
> and coda from a quick grep.
>
> ? ? ? ? ? ? ? ? ? ? ? ?Linus
>
> On Thu, May 12, 2011 at 8:04 AM, Miklos Szeredi <[email protected]> wrote:
>>
>> Please pull from
>>
>> ?git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus
>>
>> Miklos Szeredi (1):
>> ? ? ?fuse: fix oops in revalidate when called with NULL nameidata
>>
>> ---
>> ?fs/fuse/dir.c | ? ?2 +-
>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>
>
On Thu, 2011-05-12 at 08:19 -0700, Linus Torvalds wrote:
> Ooh.
>
> It's "lookup_one_len()", isn't it?
>
> So any filesystem that uses that helper will need to be protected from
> a NULL 'nd'.
>
> And then you have nfsd, ecryptfs and cachefiles that will do it on
> _other_ filesystems.
>
> Gaah. Ugly. So either we really should fix the filesystems that don't
> have protection from a NULL nd, or we should fix lookup_one_len() (are
> there perhaps other cases I missed?)
As long as we have filesystems that need to rely on intents and that
have the ability to generate automount points, we should deprecate the
use of lookup_one_len() and add something else to help ecryptfs and co
to do safe component-wise lookups, opens and creates.
The point is that filesystems such as Fuse and NFSv4 rely on intents for
opens and file creation; even NFSv3 relies on it for doing safe O_EXCL
creates. For NFSv4, the struct nameidata carries not only the path
information, but also returns the fully initialised 'struct file'. Al
has plans to clean up that interface, but lookup_one_len() will still be
insufficient as a replacement.
NFS also requires look up using the full 'struct path' if/when we happen
upon an automount point (i.e. if we cross into a submounted filesystem
on the server or if we encounter an NFSv4 referral). Again, this is
something that lookup_one_len() can't do.
Trond
> Al? Christoph? Comments?
>
> Linus
>
> On Thu, May 12, 2011 at 8:12 AM, Linus Torvalds
> <[email protected]> wrote:
> > Hmm.
> >
> > Do we really ever have a NULL 'nd' these days? Can you send me the
> > backtrace for whatever oops that was reported?
> >
> > The reason I ask is because at least NFS also does just
> >
> > if (nd->flags & LOOKUP_RCU)
> > return -ECHILD;
> >
> > in its nfs_lookup/open_revalidate() functions. As does cifs, ncpfs, p9
> > and coda from a quick grep.
> >
> > Linus
> >
> > On Thu, May 12, 2011 at 8:04 AM, Miklos Szeredi <[email protected]> wrote:
> >>
> >> Please pull from
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-linus
> >>
> >> Miklos Szeredi (1):
> >> fuse: fix oops in revalidate when called with NULL nameidata
> >>
> >> ---
> >> fs/fuse/dir.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Thu, May 12, 2011 at 9:28 AM, Trond Myklebust
<[email protected]> wrote:
>
> NFS also requires look up using the full 'struct path' if/when we happen
> upon an automount point (i.e. if we cross into a submounted filesystem
> on the server or if we encounter an NFSv4 referral). Again, this is
> something that lookup_one_len() can't do.
Sure. But we might not want to oops. Would you be willing to try
ecryptfs over NFS to at least make it limp along?
Linus
On Thu, May 12, 2011 at 10:21:21AM -0700, Linus Torvalds wrote:
> On Thu, May 12, 2011 at 9:28 AM, Trond Myklebust
> <[email protected]> wrote:
> >
> > NFS also requires look up using the full 'struct path' if/when we happen
> > upon an automount point (i.e. if we cross into a submounted filesystem
> > on the server or if we encounter an NFSv4 referral). Again, this is
> > something that lookup_one_len() can't do.
>
> Sure. But we might not want to oops. Would you be willing to try
> ecryptfs over NFS to at least make it limp along?
IMO that's crap; not the part about not wanting an oops, but lookup_one_len()
one. We do *NOT* want to pass nameidata all over the place; that had been
a mistake in the first place. Proper solution will have to wait one more
cycle, but it's going to be along the lines of "pass struct file * explicitly
on the last component handling; screw nameidata for everything else for old
methods". We already have the call sites (almost) separated in namei.c and
I'm going to finish that come the next cycle. Again, the long-term solution
will have ->d_revalidate() and ->lookup() *lose* struct nameidata * argument,
with new method used in place of ->open() for the last component. Defaulting
to the current sequence, yes, but NFS will be a lot more comfortable using
it instead of the Cthulhu-scaring kludges it relies upon now. At the same
point ecryptfs will have one more method to pass through to the underlying
fs. And nameidata will be passed only to ->follow_link(), where it's
inevitable. I hoped to do (and debug) that during the last cycle, but
do_last() got mutilated by Nick's patchset and during this cycle I was
MIA completely. Sorry, but that'll have to wait until .40 ;-/
Again, lookup_one_len() (and not having nameidata anywhere in sight) is
right and proper.
On Thu, 2011-05-12 at 10:21 -0700, Linus Torvalds wrote:
> On Thu, May 12, 2011 at 9:28 AM, Trond Myklebust
> <[email protected]> wrote:
> >
> > NFS also requires look up using the full 'struct path' if/when we happen
> > upon an automount point (i.e. if we cross into a submounted filesystem
> > on the server or if we encounter an NFSv4 referral). Again, this is
> > something that lookup_one_len() can't do.
>
> Sure. But we might not want to oops. Would you be willing to try
> ecryptfs over NFS to at least make it limp along?
>
> Linus
I was thinking rather of just returning an EIO if people try. We know
that there are breakages w.r.t. certain operations as I pointed out in
my previous email.
I suppose it really boils down to the following:
Is there really a community of people out there relying on being able to
do ecryptfs over NFS today?
If so, what are they using it for, and are they going to care about
those breakages?
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Thu, May 12, 2011 at 10:46 AM, Trond Myklebust
<[email protected]> wrote:
>
> I was thinking rather of just returning an EIO if people try. We know
> that there are breakages w.r.t. certain operations as I pointed out in
> my previous email.
Umm. Returning an error is ok, but only if you _need_ to return an error.
Returning an error because you don't like it - that's just wrong.
So I would seriously suggest you return an error only when you really
need something from the nd. So I'd suggest
- for the automount case, return an error, since you can't change the filp
- for the LOOKUP_RCU case, just do the "nd && (nd->flags & LOOKUP_RCU)"
The really doesn't seem to be many cases that really need the nd, so
why make it a hard error? *Most* users of nd already check for NULL,
either directly or indirectly (ie "if (!is_atomic_open(nd)) goto
no_open")
Linus
On Thu, 2011-05-12 at 18:40 +0100, Al Viro wrote:
> On Thu, May 12, 2011 at 10:21:21AM -0700, Linus Torvalds wrote:
> > On Thu, May 12, 2011 at 9:28 AM, Trond Myklebust
> > <[email protected]> wrote:
> > >
> > > NFS also requires look up using the full 'struct path' if/when we happen
> > > upon an automount point (i.e. if we cross into a submounted filesystem
> > > on the server or if we encounter an NFSv4 referral). Again, this is
> > > something that lookup_one_len() can't do.
> >
> > Sure. But we might not want to oops. Would you be willing to try
> > ecryptfs over NFS to at least make it limp along?
>
> IMO that's crap; not the part about not wanting an oops, but lookup_one_len()
> one. We do *NOT* want to pass nameidata all over the place; that had been
> a mistake in the first place. Proper solution will have to wait one more
> cycle, but it's going to be along the lines of "pass struct file * explicitly
> on the last component handling; screw nameidata for everything else for old
> methods". We already have the call sites (almost) separated in namei.c and
> I'm going to finish that come the next cycle. Again, the long-term solution
> will have ->d_revalidate() and ->lookup() *lose* struct nameidata * argument,
> with new method used in place of ->open() for the last component. Defaulting
> to the current sequence, yes, but NFS will be a lot more comfortable using
> it instead of the Cthulhu-scaring kludges it relies upon now. At the same
> point ecryptfs will have one more method to pass through to the underlying
> fs. And nameidata will be passed only to ->follow_link(), where it's
> inevitable. I hoped to do (and debug) that during the last cycle, but
> do_last() got mutilated by Nick's patchset and during this cycle I was
> MIA completely. Sorry, but that'll have to wait until .40 ;-/
>
> Again, lookup_one_len() (and not having nameidata anywhere in sight) is
> right and proper.
Sure. I don't care about nameidata in itself. I care about knowing when
it is appropriate to do just a lookup, when to do an atomic open or
create+open. lookup_one_len() only works for the lookup case.
My point is rather that AFAICS ecryptfs needs a different VFS helper so
that it can request 'lookup+open this component' or 'create+open this
component'. It shouldn't have to care about the details of how the
underlying filesystem works (whether that filesystem uses intents, your
new atomic open interface or does separate lookup+create+open calls).
It may also need to call follow_managed(). Again, if that is the case,
then we would need a VFS helper.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com