lkml.org 
[lkml]   [1999]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: defvs patch v84 for linux 2.2.0-pre9 bugfix
Oops. I see you posted separately to me and linux-kernel. I'm
forwarding my reply for the benefit of kernel readers.

Heinz Mauelshagen writes:
> > > I found a little bug, which seems to cause non standard block devices
> > > beeing _not_ mountable any more.
> > > In detail, my logical volume manager block devices don't work.
> > > The block device specials are created by lvm user commands.
> > >
> > > I think any software creating block specials should fail with the
> > > v84 code in super.c, where your patch looks like:
> > >
> > > @@ -1067,8 +1079,9 @@
> > > if (MAJOR(dev) >= MAX_BLKDEV)
> > > goto dput_and_out;
> > >
> > > - retval = -ENOTBLK;
> > > - dummy.f_op = get_blkfops(MAJOR(dev));
> > > + retval = devfs_fill_file (inode, &dummy, NULL);
> > > + if ( !retval && !S_ISBLK (inode->i_mode) ) retval = -ENOTBLK;
> > > + if (retval < 0) dummy.f_op = get_blkfops(MAJOR(dev));
> > > if (!dummy.f_op)
> > > goto dput_and_out;
> >
> > Can you please explain why you think my patch is not working?
>
> Please see below.
>
> >
> > Also, please send me the output of ls -lF on the device node you are
> > trying to mount.
>
> brw-r----- 1 root root 58, 1 Jan 24 03:48 /dev/vg00/u1

Has this been created from user space?

> > I do have one theory why my patch is failing. See the line:
> > if ( !retval && !S_ISBLK (inode->i_mode) ) retval = -ENOTBLK;
> > ^^
> > If the device is non-standard (i.e. the device node was created with
> > mknod(2) and not internally by the driver calling devfs_register()),
> > *and* the previous contents of the inode were for a block device, then
> > the condition fails. This means that reval will not be set to -ENOTBLK
> > and the fops are subsequently not filled. Hence you can't mount.
> > This is a braino on my part.
> >
> > I suggest changing the "&&" to a "||". This should fix your problem
> > and also provides the desired behaviour. Please let me know if this
> > works for you.
>
> That's o.k. for me, but why do you test for block device again anyway?
> It's already tested a couple of lines above in linux/fs/super.c based on
> the actual dentry (line 1343).

Ah, I had overlook that. Thanks for pointing that out. That whole line
does no good and only does harm.

> > > Patch against stock linux-2.2.0-pre9/fs/super.c to fix the problem
> > > follows:
> >
> > In future, could you please provide patches against kernel+devfs,
> > rather than providing a replacement devfs patch? This makes it easier
> > for me to understand what you're doing and also makes it easier to
> > integrate a patch.
>
> I thought i had done this 8*(

No, you provided a patch against stock linux-2.2.0-pre9 (as you
said). What it easier for me is a patch against stock
linux-2.2.0pre9+devfs-patch-v86.

> > > @@ -1067,8 +1079,9 @@
> > > if (MAJOR(dev) >= MAX_BLKDEV)
> > > goto dput_and_out;
> > >
> > > - retval = -ENOTBLK;
> > > - dummy.f_op = get_blkfops(MAJOR(dev));
> > > + if ( !( retval = devfs_fill_file (inode, &dummy, NULL)))
> > > + retval = -ENOTBLK;
> > > + if ( retval < 0) dummy.f_op = get_blkfops(MAJOR(dev));
> > > if (!dummy.f_op)
> > > goto dput_and_out;
> > >
> >
> > What you have done here is removed the check for a
> > block device returned from devfs. So now, the user could attempt to
> > mount a character device. I don't think this is a good fix to the
> > problem. See above for a suggested fix.
>
> No, i don't think so because it has already been tested before
> (see my arguments above).

The code is more subtle than I first thought. Upon further
examination, I see that your patch will break mounting devices which
do not register themselves with register_blkdev() when the
"devfs=only" boot option is passed. For example, the SCSI disc driver
calls devfs_register_blkdev() which is a no-op when "devfs=only".

I have come up with a different fix which I think is correct. I've
appended a fresh patch against stock 2.2.0-pre9. Please try this and
let me know how it goes.

> P.S.: i found another flaw. If someone does chmod a devfs block special
> sys_chmod/sys_fchmod in linux/fs/open.c updates the dentry but
> devfs internal cached mode for the inode never changes.
> If the block special contains a filesystem and you do mount/umount
> it, your changed permissions are gone afterwards.

This is intentional. Permission changes are
filesystem-specific. Imagine you have a chroot() gaol and you want to
change some permissions there but you don't want to affect any other
mounted devfs'. The current behaviour supports this.

When you unmount a devfs, you lose all the permissions that were
cached. The current way to fix this is to manaully change the
permissions when you mount a devfs.

In the long term, devfs will peek through to the underlying disc-based
filesystem and use the permissions specified there. If you change
permissions in devfs, the changes will be written back to the
underlying disc-based filesystem.

Regards,

Richard....

begin 644 devfs-patch-current.gz
[Snipped to save electrons. Anyone desperate for this before the next
official devfs patch can get it from me]

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:49    [W:0.053 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site