Date: Mon, 23 Aug 2010 16:32:58 +0200 From: Ian FREISLICH <ianf@clue.co.za> To: Kostik Belousov <kostikbel@gmail.com> Cc: Ed Schouten <ed@80386.nl>, freebsd-current@freebsd.org Subject: Re: fusefs-kmod broken? Message-ID: <E1OnY55-0001YZ-0L@clue.co.za> In-Reply-To: <20100823140149.GG2396@deviant.kiev.zoral.com.ua> References: <20100823140149.GG2396@deviant.kiev.zoral.com.ua> <201008230826.49509.jhb@freebsd.org> <E1OmUBI-0000Oy-J5@clue.co.za> <E1OnWc7-0001Kv-47@clue.co.za> <20100823132551.GE2396@deviant.kiev.zoral.com.ua> <20100823133555.GA64651@hoeg.nl> <20100823134459.GF2396@deviant.kiev.zoral.com.ua> <20100823134723.GC64651@hoeg.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
Kostik Belousov wrote:
>
> --7hK5U8dVDlZxii7z
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
>
> On Mon, Aug 23, 2010 at 03:47:23PM +0200, Ed Schouten wrote:
> > * Kostik Belousov <kostikbel@gmail.com> wrote:
> > > On Mon, Aug 23, 2010 at 03:35:55PM +0200, Ed Schouten wrote:
> > > > * Kostik Belousov <kostikbel@gmail.com> wrote:
> > > > > Which most likely means that fusesfs filled its own struct fileops
> > > > > without properly initializing fo_truncate member.
> > > >=20
> > > > It's a bit misleading that cdevs automatically patch the table, while
> > > > the fileops don't. Maybe it would be a good idea to patch finit() to
> > > I do not understand your first sentence. Would you please elaborate ?
> >=20
> > Say, you create a cdev, if you don't implement all ops, it will check
> > for null pointers and return error codes accordingly. This doesn't
> > happen for fileops, which is probably one of the reasons why people
> > sometimes forget to implement them.
> >=20
> > Wouldn't it be better to prevent this form of footshooting by adding
> > assertions? This will add some overhead for any file descriptor created,
> > but a kernel with INVARIANTS isn't meant to be fast.
> Thanks, I see it now.
>
> The cdev interface definitely falls into the public kernel interface.
> Having to fill all cdevsw methods for a random driver is too much
> burden put on the several dozens maintainers.
>
> On the other hand, file level is not much widely used by third-party
> components, and even in-tree code implements only ten different file
> types.
>
> I would not object loudly if someone put such checks as proposed
> under INVARIANTS, but also I do not see a real point in having them.
> Might be slightly better to put the checks, again under INVARIANTS,
> in the fo_XXX() wrappers.
So, in this case is the fusefs module broken? I'm guessing it is.
I don't like the way fuse_fileops is initialised in fuse4bsd. I
would prefer for the struct to be zeroed and then the fo_xxx
implimented bits set as appropriate. That way when the struct is
changed, you don't get stung again.
Patch attached to that makes fusefs-kmod not blowup kernels post this change.
Ian
--
Ian Freislich
Index: files/patch-fuse_module__fuse_vnops.c
===================================================================
RCS file: /home/ncvs/ports/sysutils/fusefs-kmod/files/patch-fuse_module__fuse_vnops.c,v
retrieving revision 1.4
diff -u -d -r1.4 patch-fuse_module__fuse_vnops.c
--- files/patch-fuse_module__fuse_vnops.c 30 Oct 2008 15:36:35 -0000 1.4
+++ files/patch-fuse_module__fuse_vnops.c 23 Aug 2010 14:27:17 -0000
+@@ -214,6 +214,7 @@
+ * following fields are filled from vnops, but "vnops.foo" is not
+ * legitimate in a definition, so we set them at module load time
+ */
++ .fo_truncate = NULL,
+ .fo_ioctl = NULL,
+ .fo_poll = NULL,
+ .fo_kqfilter = NULL,
+@@ -799,8 +800,11 @@
struct vnode *vp = ap->a_vp;
struct vattr *vap = ap->a_vap;
struct ucred *cred = ap->a_cred;
@@ -13,7 +21,7 @@
struct fuse_dispatcher fdi;
struct timespec uptsp;
int err = 0;
-@@ -871,7 +874,11 @@
+@@ -871,7 +875,11 @@
fuse_access(ap)
struct vop_access_args /* {
struct vnode *a_vp;
@@ -25,7 +33,7 @@
struct ucred *a_cred;
struct thread *a_td;
} */ *ap;
-@@ -886,7 +893,13 @@
+@@ -886,7 +894,13 @@
else
facp.facc_flags |= FACCESS_DO_ACCESS;
@@ -40,7 +48,7 @@
}
/*
-@@ -946,7 +959,11 @@
+@@ -946,7 +960,11 @@
/* We are to do the check in-kernel */
if (! (facp->facc_flags & FACCESS_VA_VALID)) {
@@ -53,7 +61,7 @@
if (err)
return (err);
facp->facc_flags |= FACCESS_VA_VALID;
-@@ -1929,7 +1946,11 @@
+@@ -1929,7 +1947,11 @@
* It will not invalidate pages which are dirty, locked, under
* writeback or mapped into pagetables.")
*/
@@ -65,7 +73,7 @@
fufh->flags |= FOPEN_KEEP_CACHE;
}
-@@ -3005,8 +3026,11 @@
+@@ -3005,8 +3027,11 @@
struct vattr *vap = ap->a_vap;
struct vnode *vp = ap->a_vp;
struct ucred *cred = ap->a_cred;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E1OnY55-0001YZ-0L>