From owner-freebsd-current@FreeBSD.ORG Mon Aug 23 14:34:35 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 30B361065694 for ; Mon, 23 Aug 2010 14:34:35 +0000 (UTC) (envelope-from ianf@clue.co.za) Received: from inbound01.jnb1.gp-online.net (inbound01.jnb1.gp-online.net [41.161.16.135]) by mx1.freebsd.org (Postfix) with ESMTP id 6967E8FC21 for ; Mon, 23 Aug 2010 14:34:34 +0000 (UTC) Received: from [41.154.88.19] (helo=clue.co.za) by inbound01.jnb1.gp-online.net with esmtpsa (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1OnY6Z-0002R9-UR; Mon, 23 Aug 2010 16:34:32 +0200 Received: from localhost ([127.0.0.1] helo=clue.co.za) by clue.co.za with esmtp (Exim 4.72 (FreeBSD)) (envelope-from ) id 1OnY55-0001YZ-0L; Mon, 23 Aug 2010 16:32:59 +0200 Message-Id: To: Kostik Belousov From: Ian FREISLICH In-Reply-To: <20100823140149.GG2396@deviant.kiev.zoral.com.ua> References: <20100823140149.GG2396@deviant.kiev.zoral.com.ua> <201008230826.49509.jhb@freebsd.org> <20100823132551.GE2396@deviant.kiev.zoral.com.ua> <20100823133555.GA64651@hoeg.nl> <20100823134459.GF2396@deviant.kiev.zoral.com.ua> <20100823134723.GC64651@hoeg.nl> X-Attribution: BOFH Date: Mon, 23 Aug 2010 16:32:58 +0200 Cc: Ed Schouten , freebsd-current@freebsd.org Subject: Re: fusefs-kmod broken? X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Aug 2010 14:34:35 -0000 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 wrote: > > > On Mon, Aug 23, 2010 at 03:35:55PM +0200, Ed Schouten wrote: > > > > * Kostik Belousov 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;