kern/159663: sockets don't work though nullfs mounts
Mikolaj Golub
trociny at freebsd.org
Sun Sep 25 22:30:19 UTC 2011
The following reply was made to PR kern/159663; it has been noted by GNATS.
From: Mikolaj Golub <trociny at freebsd.org>
To: Robert Millan <rmh at freebsd.org>
Cc: FreeBSD-gnats-submit at freebsd.org, freebsd-bugs at freebsd.org, Kostik Belousov <kostikbel at gmail.com>, Josef Karthauser <joe at freebsd.org>, Adrian Chadd <adrian at freebsd.org>, freebsd-fs at freebsd.org
Subject: Re: kern/159663: sockets don't work though nullfs mounts
Date: Mon, 26 Sep 2011 00:58:03 +0300
--=-=-=
Hi,
On Sun, 25 Sep 2011 17:32:27 +0200 Robert Millan wrote:
RM> 2011/9/24 Robert Millan <rmh at freebsd.org>:
>> I found a thread from 2007 with further discussion about this problem:
>>
>> http://lists.freebsd.org/pipermail/freebsd-fs/2007-February/002669.html
RM> Hi,
RM> I've looked at the situation in a bit more detail, for now only with
RM> sockets in mind (not named pipes). My understanding is (please
RM> correct me if I'm wrong):
RM> - nullfs holds reference counts for each vnode, but sockets have their
RM> own mechanism for reference counting (so_count / soref / sorele).
RM> vnode reference counting doesn't protect against socket being closed,
RM> which would leave a stale pointer in the upper nullfs layer.
RM> - Increasing the reference count of the socket itself can't be done in
RM> null_nodeget() because this function is merely a getter whose call
RM> doesn't indicate any meaningful event.
RM> - It's not clear to me that there's any event in time where the socket
RM> reference can be increased. If mounting a nullfs were that event,
RM> then all existing sockets would be soref'ed but we wouldn't be
RM> soref'ing future sockets created in the lower layer after the mount.
RM> This doesn't seem correct.
RM> - Possible solution: null_nodeget() semantics are replaced with
RM> something that actually allows vnodes in the upper layer to be created
RM> and destroyed.
RM> - Possible solution: upper layer has a memory structure to keep track
RM> of which sockets in the lower layer have been soref'ed.
It looks like there is no need in setting vp->v_un = lowervp->v_un for
VFIFO. They work without this modification bypassing vnode operations to lover
node and lowervp->v_un is used.
The issue is only with local sockets, because when bind or connnect is called
for nullfs file the upper v_un is used.
For me the approach "vp->v_un = lowervp->v_un" has many complications. May be
it is much easier to use always only lower vnode? What we need for this is to
make bind and connect get the lower vnode when they are called on nullfs file.
As a proof of concept below is a patch that implements it. Currently I am not
sure that vrele/vref magic is done properly, but it looks like it works for
me.
The issues with this approach I see so far:
- we need an additional flag for namei;
- nullfs can be unmounted with a socket file still being opened.
--
Mikolaj Golub
--=-=-=
Content-Type: text/x-patch
Content-Disposition: inline; filename=nullfs.sockets.patch
Index: sys/sys/namei.h
===================================================================
--- sys/sys/namei.h (revision 225716)
+++ sys/sys/namei.h (working copy)
@@ -149,7 +149,8 @@ struct nameidata {
#define AUDITVNODE1 0x04000000 /* audit the looked up vnode information */
#define AUDITVNODE2 0x08000000 /* audit the looked up vnode information */
#define TRAILINGSLASH 0x10000000 /* path ended in a slash */
-#define PARAMASK 0x1ffffe00 /* mask of parameter descriptors */
+#define LOWERVNODE 0x20000000 /* if it is a stackable fs return lower vnode */
+#define PARAMASK 0x3ffffe00 /* mask of parameter descriptors */
#define NDHASGIANT(NDP) (((NDP)->ni_cnd.cn_flags & GIANTHELD) != 0)
Index: sys/kern/uipc_usrreq.c
===================================================================
--- sys/kern/uipc_usrreq.c (revision 225716)
+++ sys/kern/uipc_usrreq.c (working copy)
@@ -493,7 +493,7 @@ uipc_bind(struct socket *so, struct sockaddr *nam,
restart:
vfslocked = 0;
- NDINIT(&nd, CREATE, MPSAFE | NOFOLLOW | LOCKPARENT | SAVENAME,
+ NDINIT(&nd, CREATE, MPSAFE | NOFOLLOW | LOCKPARENT | SAVENAME | LOWERVNODE,
UIO_SYSSPACE, buf, td);
/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */
error = namei(&nd);
@@ -1268,7 +1268,7 @@ unp_connect(struct socket *so, struct sockaddr *na
UNP_PCB_UNLOCK(unp);
sa = malloc(sizeof(struct sockaddr_un), M_SONAME, M_WAITOK);
- NDINIT(&nd, LOOKUP, MPSAFE | FOLLOW | LOCKLEAF, UIO_SYSSPACE, buf,
+ NDINIT(&nd, LOOKUP, MPSAFE | FOLLOW | LOCKLEAF | LOWERVNODE, UIO_SYSSPACE, buf,
td);
error = namei(&nd);
if (error)
Index: sys/fs/nullfs/null_vnops.c
===================================================================
--- sys/fs/nullfs/null_vnops.c (revision 225756)
+++ sys/fs/nullfs/null_vnops.c (working copy)
@@ -365,16 +365,40 @@ null_lookup(struct vop_lookup_args *ap)
vrele(lvp);
} else {
error = null_nodeget(dvp->v_mount, lvp, &vp);
- if (error)
+ if (error) {
vput(lvp);
- else
- *ap->a_vpp = vp;
+ } else if ((flags & LOWERVNODE) != 0) {
+ vref(lvp);
+ vrele(vp);
+ *ap->a_vpp = lvp;
+ } else {
+ *ap->a_vpp = vp;
+ }
}
}
return (error);
}
static int
+null_create(struct vop_create_args *ap)
+{
+ struct componentname *cnp = ap->a_cnp;
+ int flags = cnp->cn_flags;
+ int retval;
+ struct vnode *vp, *lvp;
+
+ retval = null_bypass(&ap->a_gen);
+ if (retval == 0 && (flags & LOWERVNODE) != 0) {
+ vp = *ap->a_vpp;
+ lvp = NULLVPTOLOWERVP(vp);
+ vref(lvp);
+ vrele(vp);
+ *ap->a_vpp = lvp;
+ }
+ return (retval);
+}
+
+static int
null_open(struct vop_open_args *ap)
{
int retval;
@@ -826,6 +850,7 @@ struct vop_vector null_vnodeops = {
.vop_accessx = null_accessx,
.vop_advlockpurge = vop_stdadvlockpurge,
.vop_bmap = VOP_EOPNOTSUPP,
+ .vop_create = null_create,
.vop_getattr = null_getattr,
.vop_getwritemount = null_getwritemount,
.vop_inactive = null_inactive,
--=-=-=--
More information about the freebsd-bugs
mailing list