From: Jan Blunck <[email protected]>
Userspace isn't ready for handling another file type, so silently drop
whiteout directory entries before they leave the kernel.
Signed-off-by: Jan Blunck <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
Signed-off-by: Valerie Aurora <[email protected]>
Cc: [email protected]
Cc: "J. Bruce Fields" <[email protected]>
Cc: Neil Brown <[email protected]>
---
fs/compat.c | 9 +++++++++
fs/nfsd/nfs3xdr.c | 5 +++++
fs/nfsd/nfs4xdr.c | 5 +++++
fs/nfsd/nfsxdr.c | 4 ++++
fs/readdir.c | 9 +++++++++
5 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/fs/compat.c b/fs/compat.c
index 4b6ed03..adec661 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -839,6 +839,9 @@ static int compat_fillonedir(void *__buf, const char *name, int namlen,
struct compat_old_linux_dirent __user *dirent;
compat_ulong_t d_ino;
+ if (d_type == DT_WHT)
+ return 0;
+
if (buf->result)
return -EINVAL;
d_ino = ino;
@@ -910,6 +913,9 @@ static int compat_filldir(void *__buf, const char *name, int namlen,
compat_ulong_t d_ino;
int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(compat_long_t));
+ if (d_type == DT_WHT)
+ return 0;
+
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
@@ -999,6 +1005,9 @@ static int compat_filldir64(void * __buf, const char * name, int namlen, loff_t
int reclen = ALIGN(jj + namlen + 1, sizeof(u64));
u64 off;
+ if (d_type == DT_WHT)
+ return 0;
+
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 2a533a0..9b96f5a 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -885,6 +885,11 @@ encode_entry(struct readdir_cd *ccd, const char *name, int namlen,
int elen; /* estimated entry length in words */
int num_entry_words = 0; /* actual number of words */
+ if (d_type == DT_WHT) {
+ cd->common.err = nfs_ok;
+ return 0;
+ }
+
if (cd->offset) {
u64 offset64 = offset;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 34ccf81..2ddf144 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2269,6 +2269,11 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
return 0;
}
+ if (d_type == DT_WHT) {
+ cd->common.err = nfs_ok;
+ return 0;
+ }
+
if (cd->offset)
xdr_encode_hyper(cd->offset, (u64) offset);
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 4ce005d..0e57d4b 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -503,6 +503,10 @@ nfssvc_encode_entry(void *ccdv, const char *name,
namlen, name, offset, ino);
*/
+ if (d_type == DT_WHT) {
+ cd->common.err = nfs_ok;
+ return 0;
+ }
if (offset > ~((u32) 0)) {
cd->common.err = nfserr_fbig;
return -EINVAL;
diff --git a/fs/readdir.c b/fs/readdir.c
index 7723401..3a48491 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -77,6 +77,9 @@ static int fillonedir(void * __buf, const char * name, int namlen, loff_t offset
struct old_linux_dirent __user * dirent;
unsigned long d_ino;
+ if (d_type == DT_WHT)
+ return 0;
+
if (buf->result)
return -EINVAL;
d_ino = ino;
@@ -154,6 +157,9 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
unsigned long d_ino;
int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(long));
+ if (d_type == DT_WHT)
+ return 0;
+
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
@@ -239,6 +245,9 @@ static int filldir64(void * __buf, const char * name, int namlen, loff_t offset,
struct getdents_callback64 * buf = (struct getdents_callback64 *) __buf;
int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 1, sizeof(u64));
+ if (d_type == DT_WHT)
+ return 0;
+
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
--
1.6.3.3
On Fri, May 07, 2010 at 07:18:08AM +1000, Neil Brown wrote:
> On Thu, 6 May 2010 14:01:51 -0400
> Valerie Aurora <[email protected]> wrote:
>
> > On Tue, May 04, 2010 at 09:37:31AM +1000, Neil Brown wrote:
> > > On Mon, 3 May 2010 16:12:04 -0700
> > > Valerie Aurora <[email protected]> wrote:
> > >
> > > > From: Jan Blunck <[email protected]>
> > > >
> > > > Userspace isn't ready for handling another file type, so silently drop
> > > > whiteout directory entries before they leave the kernel.
> > >
> > > Feels very intrusive doesn't it....
> > >
> > > Have you considered something like the following?
> >
> > Hrm, I see how that could be more elegant, but I'd rather avoid yet
> > another layer of function pointer passing around. This code is
> > already hard enough to review...
>
> Yes, the extra indirection is a bit of a negative, but I don't think this
> patch is harder to review than the alternate.
> From a numerical perspective, with this patch you only need to look at the
> various places that ->readdir is called to be sure it is always correct.
> There are about 3. With the original you need to look at ever filldir
> function. Jan has found 9.
>
> And from a maintainability perspective, I think my approach is safer. Given
> that there are 9 filldir functions already, the chance that a need will be
> found for another seems good, and the chance that the coder will know to
> check for DT_WHT is a best even. Conversely if another call to ->readdir
> were added it is likely that nothing would need to be done.
>
> Of course just counting things doesn't give a completely picture but I think
> it can be indicative.
Okay, good points. Let me try it out after getting this next rewrite done.
Thanks,
-VAL
On Mon, 3 May 2010 16:12:04 -0700
Valerie Aurora <[email protected]> wrote:
> From: Jan Blunck <[email protected]>
>
> Userspace isn't ready for handling another file type, so silently drop
> whiteout directory entries before they leave the kernel.
Feels very intrusive doesn't it....
Have you considered something like the following?
NeilBrown
diff --git a/fs/readdir.c b/fs/readdir.c
index 7723401..4c5b347 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -19,10 +19,26 @@
#include <asm/uaccess.h>
+struct readdir_info {
+ filldir_t filler;
+ void *data;
+};
+
+static int white_out(void *vrdi, const char *name, int namlen,
+ loff_t offset, u64 ino, unsigned int d_type)
+{
+ struct readdir_info *rdi = vrdi;
+ if (d_type == DT_WHT)
+ return 0;
+ return rdi->filler(rdi->data, name, namlen, offset, info, d_type);
+}
+
int vfs_readdir(struct file *file, filldir_t filler, void *buf)
{
struct inode *inode = file->f_path.dentry->d_inode;
int res = -ENOTDIR;
+ struct readir_info rdi = { filler, buf };
+
if (!file->f_op || !file->f_op->readdir)
goto out;
@@ -36,7 +52,7 @@ int vfs_readdir(struct file *file, filldir_t filler, void *buf)
res = -ENOENT;
if (!IS_DEADDIR(inode)) {
- res = file->f_op->readdir(file, buf, filler);
+ res = file->f_op->readdir(file, &rdi, white_out);
file_accessed(file);
}
mutex_unlock(&inode->i_mutex);
On Tue, May 04, 2010 at 09:37:31AM +1000, Neil Brown wrote:
> On Mon, 3 May 2010 16:12:04 -0700
> Valerie Aurora <[email protected]> wrote:
>
> > From: Jan Blunck <[email protected]>
> >
> > Userspace isn't ready for handling another file type, so silently drop
> > whiteout directory entries before they leave the kernel.
>
> Feels very intrusive doesn't it....
>
> Have you considered something like the following?
Hrm, I see how that could be more elegant, but I'd rather avoid yet
another layer of function pointer passing around. This code is
already hard enough to review...
-VAL
> NeilBrown
>
> diff --git a/fs/readdir.c b/fs/readdir.c
> index 7723401..4c5b347 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -19,10 +19,26 @@
>
> #include <asm/uaccess.h>
>
> +struct readdir_info {
> + filldir_t filler;
> + void *data;
> +};
> +
> +static int white_out(void *vrdi, const char *name, int namlen,
> + loff_t offset, u64 ino, unsigned int d_type)
> +{
> + struct readdir_info *rdi = vrdi;
> + if (d_type == DT_WHT)
> + return 0;
> + return rdi->filler(rdi->data, name, namlen, offset, info, d_type);
> +}
> +
> int vfs_readdir(struct file *file, filldir_t filler, void *buf)
> {
> struct inode *inode = file->f_path.dentry->d_inode;
> int res = -ENOTDIR;
> + struct readir_info rdi = { filler, buf };
> +
> if (!file->f_op || !file->f_op->readdir)
> goto out;
>
> @@ -36,7 +52,7 @@ int vfs_readdir(struct file *file, filldir_t filler, void *buf)
>
> res = -ENOENT;
> if (!IS_DEADDIR(inode)) {
> - res = file->f_op->readdir(file, buf, filler);
> + res = file->f_op->readdir(file, &rdi, white_out);
> file_accessed(file);
> }
> mutex_unlock(&inode->i_mutex);
On Thu, 6 May 2010 14:01:51 -0400
Valerie Aurora <[email protected]> wrote:
> On Tue, May 04, 2010 at 09:37:31AM +1000, Neil Brown wrote:
> > On Mon, 3 May 2010 16:12:04 -0700
> > Valerie Aurora <[email protected]> wrote:
> >
> > > From: Jan Blunck <[email protected]>
> > >
> > > Userspace isn't ready for handling another file type, so silently drop
> > > whiteout directory entries before they leave the kernel.
> >
> > Feels very intrusive doesn't it....
> >
> > Have you considered something like the following?
>
> Hrm, I see how that could be more elegant, but I'd rather avoid yet
> another layer of function pointer passing around. This code is
> already hard enough to review...
Yes, the extra indirection is a bit of a negative, but I don't think this
patch is harder to review than the alternate.
From a numerical perspective, with this patch you only need to look at the
various places that ->readdir is called to be sure it is always correct.
There are about 3. With the original you need to look at ever filldir
function. Jan has found 9.
And from a maintainability perspective, I think my approach is safer. Given
that there are 9 filldir functions already, the chance that a need will be
found for another seems good, and the chance that the coder will know to
check for DT_WHT is a best even. Conversely if another call to ->readdir
were added it is likely that nothing would need to be done.
Of course just counting things doesn't give a completely picture but I think
it can be indicative.
NeilBrown
>
> -VAL
>
> > NeilBrown
> >
> > diff --git a/fs/readdir.c b/fs/readdir.c
> > index 7723401..4c5b347 100644
> > --- a/fs/readdir.c
> > +++ b/fs/readdir.c
> > @@ -19,10 +19,26 @@
> >
> > #include <asm/uaccess.h>
> >
> > +struct readdir_info {
> > + filldir_t filler;
> > + void *data;
> > +};
> > +
> > +static int white_out(void *vrdi, const char *name, int namlen,
> > + loff_t offset, u64 ino, unsigned int d_type)
> > +{
> > + struct readdir_info *rdi = vrdi;
> > + if (d_type == DT_WHT)
> > + return 0;
> > + return rdi->filler(rdi->data, name, namlen, offset, info, d_type);
> > +}
> > +
> > int vfs_readdir(struct file *file, filldir_t filler, void *buf)
> > {
> > struct inode *inode = file->f_path.dentry->d_inode;
> > int res = -ENOTDIR;
> > + struct readir_info rdi = { filler, buf };
> > +
> > if (!file->f_op || !file->f_op->readdir)
> > goto out;
> >
> > @@ -36,7 +52,7 @@ int vfs_readdir(struct file *file, filldir_t filler, void *buf)
> >
> > res = -ENOENT;
> > if (!IS_DEADDIR(inode)) {
> > - res = file->f_op->readdir(file, buf, filler);
> > + res = file->f_op->readdir(file, &rdi, white_out);
> > file_accessed(file);
> > }
> > mutex_unlock(&inode->i_mutex);