2000-11-08 15:58:19

by Tigran Aivazian

Subject: [patch-2.4.0-test11-pre1] a few more BUG()s for vfs_XXX()

Hi Linus,

I think that the check for inode->i_op == NULL in various
vfs_XXX() functions is bogus, i.e. if it is NULL then it must be a bug in
some filesystem's ->read_inode() method and therefore, instead of
returning error to userspace we should immediately panic, since it is a
kernel bug.

The patch below does just that -- i.e. converts them into BUG()s. This
also has the added advantage that if we are 100% confident in our
filesystems (or kernel in general) we can compile with #define BUG /* */
and get a smaller/faster kernel.

Regards,
Tigran

diff -urN -X dontdiff linux/fs/attr.c BUG/fs/attr.c
--- linux/fs/attr.c Mon Oct 16 21:00:53 2000
+++ BUG/fs/attr.c Wed Nov 8 15:43:35 2000
@@ -113,6 +113,9 @@
if (!inode)
BUG();

+ if (!inode->i_op)
+ BUG();
+
attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
attr->ia_atime = now;
@@ -120,7 +123,7 @@
attr->ia_mtime = now;

lock_kernel();
- if (inode->i_op && inode->i_op->setattr)
+ if (inode->i_op->setattr)
error = inode->i_op->setattr(dentry, attr);
else {
error = inode_change_ok(inode, attr);
diff -urN -X dontdiff linux/fs/namei.c BUG/fs/namei.c
--- linux/fs/namei.c Fri Sep 22 22:21:18 2000
+++ BUG/fs/namei.c Wed Nov 8 15:43:10 2000
@@ -156,7 +156,10 @@
{
int mode = inode->i_mode;

- if (inode->i_op && inode->i_op->permission) {
+ if (!inode->i_op)
+ BUG();
+
+ if (inode->i_op->permission) {
int retval;
lock_kernel();
retval = inode->i_op->permission(inode, mask);
@@ -898,13 +901,16 @@
mode &= S_IALLUGO & ~current->fs->umask;
mode |= S_IFREG;

+ if (!dir->i_op)
+ BUG();
+
down(&dir->i_zombie);
error = may_create(dir, dentry);
if (error)
goto exit_lock;

error = -EACCES; /* shouldn't it be ENOSYS? */
- if (!dir->i_op || !dir->i_op->create)
+ if (!dir->i_op->create)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1173,6 +1179,9 @@
{
int error = -EPERM;

+ if (!dir->i_op)
+ BUG();
+
mode &= ~current->fs->umask;

down(&dir->i_zombie);
@@ -1184,7 +1193,7 @@
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->mknod)
+ if (!dir->i_op->mknod)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1245,13 +1254,16 @@
{
int error;

+ if (!dir->i_op)
+ BUG();
+
down(&dir->i_zombie);
error = may_create(dir, dentry);
if (error)
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->mkdir)
+ if (!dir->i_op->mkdir)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1328,12 +1340,15 @@
int vfs_rmdir(struct inode *dir, struct dentry *dentry)
{
int error;
+
+ if (!dir->i_op)
+ BUG();

error = may_delete(dir, dentry, 1);
if (error)
return error;
-
- if (!dir->i_op || !dir->i_op->rmdir)
+
+ if (!dir->i_op->rmdir)
return -EPERM;

DQUOT_INIT(dir);
@@ -1404,11 +1419,14 @@
{
int error;

+ if (!dir->i_op)
+ BUG();
+
down(&dir->i_zombie);
error = may_delete(dir, dentry, 0);
if (!error) {
error = -EPERM;
- if (dir->i_op && dir->i_op->unlink) {
+ if (dir->i_op->unlink) {
DQUOT_INIT(dir);
if (d_mountpoint(dentry))
error = -EBUSY;
@@ -1474,13 +1492,16 @@
{
int error;

+ if (!dir->i_op)
+ BUG();
+
down(&dir->i_zombie);
error = may_create(dir, dentry);
if (error)
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->symlink)
+ if (!dir->i_op->symlink)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1534,6 +1555,9 @@
struct inode *inode;
int error;

+ if (!dir->i_op)
+ BUG();
+
down(&dir->i_zombie);
error = -ENOENT;
inode = old_dentry->d_inode;
@@ -1554,7 +1578,7 @@
error = -EPERM;
if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
goto exit_lock;
- if (!dir->i_op || !dir->i_op->link)
+ if (!dir->i_op->link)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1658,6 +1682,9 @@
if (old_dentry->d_inode == new_dentry->d_inode)
return 0;

+ if (!old_dir->i_op)
+ BUG();
+
error = may_delete(old_dir, old_dentry, 1);
if (error)
return error;
@@ -1672,7 +1699,7 @@
if (error)
return error;

- if (!old_dir->i_op || !old_dir->i_op->rename)
+ if (!old_dir->i_op->rename)
return -EPERM;

/*
@@ -1734,6 +1761,9 @@
if (old_dentry->d_inode == new_dentry->d_inode)
return 0;

+ if (!old_dir->i_op)
+ BUG();
+
error = may_delete(old_dir, old_dentry, 0);
if (error)
return error;
@@ -1748,7 +1778,7 @@
if (error)
return error;

- if (!old_dir->i_op || !old_dir->i_op->rename)
+ if (!old_dir->i_op->rename)
return -EPERM;

DQUOT_INIT(old_dir);

2000-11-08 18:34:45

by Alexander Viro

Subject: Re: [patch-2.4.0-test11-pre1] a few more BUG()s for vfs_XXX()

On Wed, 8 Nov 2000, Tigran Aivazian wrote:

> Hi Linus,
>
> I think that the check for inode->i_op == NULL in various
> vfs_XXX() functions is bogus, i.e. if it is NULL then it must be a bug in
> some filesystem's ->read_inode() method and therefore, instead of
> returning error to userspace we should immediately panic, since it is a
> kernel bug.

Frankly, I don't see the reason why ->i_op should be always non-NULL.
We may go for such variant, but right now it is legitimate. Silly - yes,
but...

> The patch below does just that -- i.e. converts them into BUG()s. This

Now, _that_ is just plain silly. They are immediately followed by dereferencing
the thing. And we don't have a lot of architecture-specific filesystems.

If we want to do it - let's do it right. Patch below should take care of
all places in the main tree and 3rd-party code should make sure that
i_op is never set to NULL. If you want "no methods" - just leave i_op
as is, fs/inode.c initializes it so that it points to empty table anyway.

Cheers,
Al

Patch follows:
diff -urN rc11-pre1/arch/ia64/kernel/sys_ia64.c rc11-pre1-i_op/arch/ia64/kernel/sys_ia64.c
--- rc11-pre1/arch/ia64/kernel/sys_ia64.c Thu Nov 2 22:38:14 2000
+++ rc11-pre1-i_op/arch/ia64/kernel/sys_ia64.c Wed Nov 8 16:41:48 2000
@@ -205,7 +205,7 @@
do_revalidate (struct dentry *dentry)
{
struct inode * inode = dentry->d_inode;
- if (inode->i_op && inode->i_op->revalidate)
+ if (inode->i_op->revalidate)
return inode->i_op->revalidate(dentry);
return 0;
}
diff -urN rc11-pre1/arch/mips64/kernel/linux32.c rc11-pre1-i_op/arch/mips64/kernel/linux32.c
--- rc11-pre1/arch/mips64/kernel/linux32.c Tue Aug 1 19:50:08 2000
+++ rc11-pre1-i_op/arch/mips64/kernel/linux32.c Wed Nov 8 16:41:58 2000
@@ -38,7 +38,7 @@
{
struct inode * inode = dentry->d_inode;

- if (inode->i_op && inode->i_op->revalidate)
+ if (inode->i_op->revalidate)
return inode->i_op->revalidate(dentry);

return 0;
diff -urN rc11-pre1/arch/sparc64/kernel/sys_sparc32.c rc11-pre1-i_op/arch/sparc64/kernel/sys_sparc32.c
--- rc11-pre1/arch/sparc64/kernel/sys_sparc32.c Thu Nov 2 22:38:20 2000
+++ rc11-pre1-i_op/arch/sparc64/kernel/sys_sparc32.c Wed Nov 8 16:42:05 2000
@@ -1544,7 +1544,7 @@
do_revalidate(struct dentry *dentry)
{
struct inode * inode = dentry->d_inode;
- if (inode->i_op && inode->i_op->revalidate)
+ if (inode->i_op->revalidate)
return inode->i_op->revalidate(dentry);
return 0;
}
diff -urN rc11-pre1/drivers/isdn/avmb1/capifs.c rc11-pre1-i_op/drivers/isdn/avmb1/capifs.c
--- rc11-pre1/drivers/isdn/avmb1/capifs.c Thu Nov 2 22:38:31 2000
+++ rc11-pre1-i_op/drivers/isdn/avmb1/capifs.c Wed Nov 8 16:42:17 2000
@@ -468,7 +468,6 @@
ino_t ino = inode->i_ino;
struct capifs_sb_info *sbi = SBI(inode->i_sb);

- inode->i_op = NULL;
inode->i_mode = 0;
inode->i_nlink = 0;
inode->i_size = 0;
diff -urN rc11-pre1/fs/attr.c rc11-pre1-i_op/fs/attr.c
--- rc11-pre1/fs/attr.c Thu Nov 2 22:38:56 2000
+++ rc11-pre1-i_op/fs/attr.c Wed Nov 8 16:42:28 2000
@@ -120,7 +120,7 @@
attr->ia_mtime = now;

lock_kernel();
- if (inode->i_op && inode->i_op->setattr)
+ if (inode->i_op->setattr)
error = inode->i_op->setattr(dentry, attr);
else {
error = inode_change_ok(inode, attr);
diff -urN rc11-pre1/fs/autofs4/inode.c rc11-pre1-i_op/fs/autofs4/inode.c
--- rc11-pre1/fs/autofs4/inode.c Thu Apr 27 22:09:53 2000
+++ rc11-pre1-i_op/fs/autofs4/inode.c Wed Nov 8 16:46:25 2000
@@ -313,7 +313,6 @@
inode->i_blocks = 0;
inode->i_rdev = 0;
inode->i_nlink = 1;
- inode->i_op = NULL;
inode->i_fop = NULL;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;

@@ -323,6 +322,8 @@
inode->i_fop = &autofs4_dir_operations;
} else if (S_ISLNK(inf->mode))
inode->i_op = &autofs4_symlink_inode_operations;
+ else
+ BUG();

return inode;
}
diff -urN rc11-pre1/fs/isofs/inode.c rc11-pre1-i_op/fs/isofs/inode.c
--- rc11-pre1/fs/isofs/inode.c Mon Jul 31 17:13:26 2000
+++ rc11-pre1-i_op/fs/isofs/inode.c Wed Nov 8 16:50:03 2000
@@ -799,9 +799,6 @@
/* check the root inode */
if (!inode)
goto out_no_root;
- if (!inode->i_op)
- goto out_bad_root;
- /* get the root dentry */
s->s_root = d_alloc_root(inode);
if (!(s->s_root))
goto out_no_root;
diff -urN rc11-pre1/fs/namei.c rc11-pre1-i_op/fs/namei.c
--- rc11-pre1/fs/namei.c Wed Oct 4 03:44:55 2000
+++ rc11-pre1-i_op/fs/namei.c Wed Nov 8 16:52:12 2000
@@ -156,7 +156,7 @@
{
int mode = inode->i_mode;

- if (inode->i_op && inode->i_op->permission) {
+ if (inode->i_op->permission) {
int retval;
lock_kernel();
retval = inode->i_op->permission(inode, mask);
@@ -503,10 +503,6 @@
inode = dentry->d_inode;
if (!inode)
goto out_dput;
- err = -ENOTDIR;
- if (!inode->i_op)
- goto out_dput;
-
if (inode->i_op->follow_link) {
err = do_follow_link(dentry, nd);
dput(dentry);
@@ -516,9 +512,6 @@
inode = nd->dentry->d_inode;
if (!inode)
break;
- err = -ENOTDIR;
- if (!inode->i_op)
- break;
} else {
dput(nd->dentry);
nd->dentry = dentry;
@@ -562,7 +555,7 @@
;
inode = dentry->d_inode;
if ((lookup_flags & LOOKUP_FOLLOW)
- && inode && inode->i_op && inode->i_op->follow_link) {
+ && inode && inode->i_op->follow_link) {
err = do_follow_link(dentry, nd);
dput(dentry);
if (err)
@@ -577,7 +570,7 @@
goto no_inode;
if (lookup_flags & LOOKUP_DIRECTORY) {
err = -ENOTDIR;
- if (!inode->i_op || !inode->i_op->lookup)
+ if (!inode->i_op->lookup)
break;
}
goto return_base;
@@ -904,7 +897,7 @@
goto exit_lock;

error = -EACCES; /* shouldn't it be ENOSYS? */
- if (!dir->i_op || !dir->i_op->create)
+ if (!dir->i_op->create)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1014,7 +1007,7 @@
error = -ENOENT;
if (!dentry->d_inode)
goto exit_dput;
- if (dentry->d_inode->i_op && dentry->d_inode->i_op->follow_link)
+ if (dentry->d_inode->i_op->follow_link)
goto do_link;

dput(nd->dentry);
@@ -1184,7 +1177,7 @@
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->mknod)
+ if (!dir->i_op->mknod)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1251,7 +1244,7 @@
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->mkdir)
+ if (!dir->i_op->mkdir)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1333,7 +1326,7 @@
if (error)
return error;

- if (!dir->i_op || !dir->i_op->rmdir)
+ if (!dir->i_op->rmdir)
return -EPERM;

DQUOT_INIT(dir);
@@ -1408,7 +1401,7 @@
error = may_delete(dir, dentry, 0);
if (!error) {
error = -EPERM;
- if (dir->i_op && dir->i_op->unlink) {
+ if (dir->i_op->unlink) {
DQUOT_INIT(dir);
if (d_mountpoint(dentry))
error = -EBUSY;
@@ -1480,7 +1473,7 @@
goto exit_lock;

error = -EPERM;
- if (!dir->i_op || !dir->i_op->symlink)
+ if (!dir->i_op->symlink)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1554,7 +1547,7 @@
error = -EPERM;
if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
goto exit_lock;
- if (!dir->i_op || !dir->i_op->link)
+ if (!dir->i_op->link)
goto exit_lock;

DQUOT_INIT(dir);
@@ -1672,7 +1665,7 @@
if (error)
return error;

- if (!old_dir->i_op || !old_dir->i_op->rename)
+ if (!old_dir->i_op->rename)
return -EPERM;

/*
@@ -1748,7 +1741,7 @@
if (error)
return error;

- if (!old_dir->i_op || !old_dir->i_op->rename)
+ if (!old_dir->i_op->rename)
return -EPERM;

DQUOT_INIT(old_dir);
diff -urN rc11-pre1/fs/nfsd/vfs.c rc11-pre1-i_op/fs/nfsd/vfs.c
--- rc11-pre1/fs/nfsd/vfs.c Wed Nov 8 16:35:26 2000
+++ rc11-pre1-i_op/fs/nfsd/vfs.c Wed Nov 8 16:49:54 2000
@@ -829,7 +829,7 @@
dirp = dentry->d_inode;

err = nfserr_notdir;
- if(!dirp->i_op || !dirp->i_op->lookup)
+ if(!dirp->i_op->lookup)
goto out;
/*
* Check whether the response file handle has been verified yet.
@@ -956,7 +956,7 @@
/* Get all the sanity checks out of the way before
* we lock the parent. */
err = nfserr_notdir;
- if(!dirp->i_op || !dirp->i_op->lookup)
+ if(!dirp->i_op->lookup)
goto out;
fh_lock(fhp);

@@ -1077,7 +1077,7 @@
inode = dentry->d_inode;

err = nfserr_inval;
- if (!inode->i_op || !inode->i_op->readlink)
+ if (!inode->i_op->readlink)
goto out;

UPDATE_ATIME(inode);
diff -urN rc11-pre1/fs/ramfs/inode.c rc11-pre1-i_op/fs/ramfs/inode.c
--- rc11-pre1/fs/ramfs/inode.c Wed Oct 4 03:44:58 2000
+++ rc11-pre1-i_op/fs/ramfs/inode.c Wed Nov 8 16:48:31 2000
@@ -123,7 +123,6 @@
inode->i_blocks = 0;
inode->i_rdev = to_kdev_t(dev);
inode->i_nlink = 1;
- inode->i_op = NULL;
inode->i_fop = NULL;
inode->i_mapping->a_ops = &ramfs_aops;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
diff -urN rc11-pre1/fs/stat.c rc11-pre1-i_op/fs/stat.c
--- rc11-pre1/fs/stat.c Mon Aug 28 21:15:19 2000
+++ rc11-pre1-i_op/fs/stat.c Wed Nov 8 16:50:21 2000
@@ -19,7 +19,7 @@
do_revalidate(struct dentry *dentry)
{
struct inode * inode = dentry->d_inode;
- if (inode->i_op && inode->i_op->revalidate)
+ if (inode->i_op->revalidate)
return inode->i_op->revalidate(dentry);
return 0;
}
@@ -255,7 +255,7 @@
struct inode * inode = nd.dentry->d_inode;

error = -EINVAL;
- if (inode->i_op && inode->i_op->readlink &&
+ if (inode->i_op->readlink &&
!(error = do_revalidate(nd.dentry))) {
UPDATE_ATIME(inode);
error = inode->i_op->readlink(nd.dentry, buf, bufsiz);
diff -urN rc11-pre1/ipc/shm.c rc11-pre1-i_op/ipc/shm.c
--- rc11-pre1/ipc/shm.c Wed Oct 4 03:45:11 2000
+++ rc11-pre1-i_op/ipc/shm.c Wed Nov 8 16:49:14 2000
@@ -376,7 +376,6 @@
struct shmid_kernel *shp;

id = inode->i_ino;
- inode->i_op = NULL;
inode->i_mode = 0;
inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;

diff -urN rc11-pre1/mm/memory.c rc11-pre1-i_op/mm/memory.c
--- rc11-pre1/mm/memory.c Wed Nov 8 16:35:28 2000
+++ rc11-pre1-i_op/mm/memory.c Wed Nov 8 16:49:30 2000
@@ -996,7 +996,7 @@
spin_unlock(&mapping->i_shared_lock);
/* this should go into ->truncate */
inode->i_size = offset;
- if (inode->i_op && inode->i_op->truncate)
+ if (inode->i_op->truncate)
inode->i_op->truncate(inode);
return;

@@ -1013,7 +1013,7 @@
}
}
inode->i_size = offset;
- if (inode->i_op && inode->i_op->truncate)
+ if (inode->i_op->truncate)
inode->i_op->truncate(inode);
out:
return;