NetBSD Problem Report #54829

From www@netbsd.org  Sat Jan  4 11:11:54 2020
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 52AC47A155
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  4 Jan 2020 11:11:54 +0000 (UTC)
Message-Id: <20200104111153.83D167A1D7@mollari.NetBSD.org>
Date: Sat,  4 Jan 2020 11:11:53 +0000 (UTC)
From: idrarig.alocin@gmail.com
Reply-To: idrarig.alocin@gmail.com
To: gnats-bugs@NetBSD.org
Subject: rename operation crashes mount_9p if target file already exists
X-Send-Pr-Version: www-1.0

>Number:         54829
>Category:       bin
>Synopsis:       rename operation crashes mount_9p if target file already exists
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 04 11:15:00 +0000 2020
>Last-Modified:  Wed May 27 00:40:01 +0000 2020
>Originator:     Nicola Girardi
>Release:        8.1
>Organization:
>Environment:
NetBSD hope 8.1 NetBSD 8.1 (GENERIC) #0: Fri May 31 08:43:59 UTC 2019  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
I was trying to use a 9P fs in NetBSD and found mount_9p crashes when
on the rename op when the target file already exists.

AFAICS the first problem is passing pn_targ->pn_data instead of
pn_targ to noderemove(), causing invalid memory access in
noderemove().

The second problem seems to be the call to puffs_setback() from the
rename handler, which fails an assertion and aborts the file server.
Since mount_psshfs does not call puffs_setback() in the analogous
situation (it also has to remove before rename if the target already
exists), I've removed the call here as well, in my proposed fix
below. I haven't studied the internals enough to be confident about
the patch and can only say that it works for me.

>How-To-Repeat:
While I found the problem by cloning a git repository on a mounted 9P
fs, this would be the minimal reproducer:

mount_9p yourhost /yourmnt
cd /yourmnt
touch a
touch b
mv a b

>Fix:
Proposed fix but needs more scrutiny.

; cvs diff -u
cvs diff: Diffing .
Index: node.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/puffs/mount_9p/node.c,v
retrieving revision 1.21
diff -u -r1.21 node.c
--- node.c      18 Jan 2009 10:10:47 -0000      1.21
+++ node.c      4 Jan 2020 10:05:31 -0000
@@ -516,11 +516,7 @@
                p9n->fid_base = P9P_INVALFID;
                puffs_pn_remove(pn);
        }
-
- out:
-       if (rv == 0)
-               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
-
+out:
        RETURN(rv);
 }

@@ -528,24 +524,34 @@
 puffs9p_node_remove(struct puffs_usermount *pu, void *opc, void *targ,
        const struct puffs_cn *pcn)
 {
+       struct puffs_cc *pcc = puffs_cc_getcc(pu);
+       int rv = 0;
        struct puffs_node *pn = targ;

        if (pn->pn_va.va_type == VDIR)
                return EISDIR;

-       return noderemove(pu, pn);
+       rv = noderemove(pu, pn);
+       if (rv == 0)
+               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
+       return rv;
 }

 int
 puffs9p_node_rmdir(struct puffs_usermount *pu, void *opc, void *targ,
        const struct puffs_cn *pcn)
 {
+       struct puffs_cc *pcc = puffs_cc_getcc(pu);
+       int rv = 0;
        struct puffs_node *pn = targ;

        if (pn->pn_va.va_type != VDIR)
                return ENOTDIR;

-       return noderemove(pu, pn);
+       rv = noderemove(pu, pn);
+       if (rv == 0)
+               puffs_setback(pcc, PUFFS_SETBACK_NOREF_N2);
+       return rv;
 }

 /*
@@ -571,7 +577,7 @@
        if (targ) {
                struct puffs_node *pn_targ = targ;

-               rv = noderemove(pu, pn_targ->pn_data);
+               rv = noderemove(pu, pn_targ);
                if (rv)
                        goto out;
        }

>Release-Note:

>Audit-Trail:

From: "Valeriy E. Ushakov" <uwe@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54829 CVS commit: src/usr.sbin/puffs/mount_9p
Date: Wed, 27 May 2020 00:05:23 +0000

 Module Name:	src
 Committed By:	uwe
 Date:		Wed May 27 00:05:22 UTC 2020

 Modified Files:
 	src/usr.sbin/puffs/mount_9p: node.c

 Log Message:
 puffs9p_node_rename - noderename takes puffs_node, not p9pnode, but
 this goes undetected b/c pn_data we pass is a pointer to void.

 From Nicola Girardi, part of PR/54829.


 To generate a diff of this commit:
 cvs rdiff -u -r1.25 -r1.26 src/usr.sbin/puffs/mount_9p/node.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Valeriy E. Ushakov" <uwe@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54829 CVS commit: src/usr.sbin/puffs/mount_9p
Date: Wed, 27 May 2020 00:36:08 +0000

 Module Name:	src
 Committed By:	uwe
 Date:		Wed May 27 00:36:08 UTC 2020

 Modified Files:
 	src/usr.sbin/puffs/mount_9p: node.c

 Log Message:
 noderemove - do not call puffs_setback here.  noderemove is called
 from rename (for existing target) and calling setback is not
 appropriate in that context.  Do that call instead directly in the
 callers (remove, rmdir).

 From Nicola Girardi, part of PR/54829.


 To generate a diff of this commit:
 cvs rdiff -u -r1.26 -r1.27 src/usr.sbin/puffs/mount_9p/node.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search