NetBSD Problem Report #48799

From www@NetBSD.org  Sat May 10 21:48:47 2014
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id F24CAA5813
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 10 May 2014 21:48:46 +0000 (UTC)
Message-Id: <20140510214845.9634EA5854@mollari.NetBSD.org>
Date: Sat, 10 May 2014 21:48:45 +0000 (UTC)
From: scdbackup@gmx.net
Reply-To: scdbackup@gmx.net
To: gnats-bugs@NetBSD.org
Subject: 32 bit ino_t bottleneck in interface of cd9660 to vfsops sabotages NFS
X-Send-Pr-Version: www-1.0

>Number:         48799
>Category:       kern
>Synopsis:       32 bit ino_t bottleneck in interface of cd9660 to vfsops sabotages NFS
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat May 10 21:50:00 +0000 2014
>Closed-Date:    Wed May 14 06:33:09 +0000 2014
>Last-Modified:  Wed May 14 06:33:09 +0000 2014
>Originator:     Thomas Schmitt
>Release:        6.1.3
>Organization:
>Environment:
NetBSD netbsd 6.1.3 NetBSD 6.1.3 (GENERIC) i386
>Description:
During my assessment of the relation between struct iso_node and
ISO 9660 File Sections, i came to the definition of the cd9660
version of vsops struct fid.
struct ifid is declared in cd9660_vfsops.c with comment
"File handle to vnode":

  struct ifid {
        ushort  ifid_len;
        ushort  ifid_pad;
        int     ifid_ino;
        long    ifid_start;
  };

Function cd9660_vptofh() copies this struct over a struct fid
which is provided by the caller.
Before this copying ifid_ino gets filled with the lower 32 bit of
an ino_t value (ip is a struct iso_node, ip->i_number is ino_t):

        ifh.ifid_ino = ip->i_number;

The higher bits are thrown away, obviously.

Function cd9660_fhtovp() copies such bytes back from struct fid
into a local struct ifid and uses .ifid_ino with

        if ((error = VFS_VGET(mp, ifh.ifid_ino, &nvp)) != 0) {

VFS_VGET to my assessment is actually function cd9660_vget_internal().
It uses cd9660_ihashget() to look up a vnode.

man 9 vfsops mentions that VFS_FHTOVP is related to NFS.
Taylor R Campbell on tech-kern teached me how to get an NFS
server running.

The attempt to produce the problem shows a sufficiently
undesirable result.


>How-To-Repeat:
Test ISO is

    http://scdbackup.webframe.org/large.iso.bz2

4470 bytes, MD5 7d78dc3efaec8ea3f1801335329f410d.
It inflates to 4,329,897,984 bytes.

Do this only if the fix of kern/48787 is applied.
If not, you will not get rid of the /mnt/iso mount point until reboot !

If the ISO is submitted as /dev/cd0, do:

  mount_cd9660 /dev/cd0a /mnt/iso
  mount_nfs localhost:/mnt/iso /mnt/nfs

Situation afterwards:

  netbsd# mount
  ...
  /dev/cd0a on /mnt/iso type cd9660 (read-only, NFS exported, local)
  localhost:/mnt/iso on /mnt/nfs type nfs
  netbsd# ls -l /mnt/nfs
  -r-xr-xr-x  1 root  wheel  0 Jan  1  1970 /mnt/nfs

Other than with kern/48787, it is possible to umount /mnt/nfs.

After the fix, expect

  netbsd# ls -l /mnt/nfs
  total 4
  drwxr-xr-x  1 thomas  dbus  2048 May  6 15:30 my
  -rw-r--r--  1 thomas  dbus     6 May  6 15:34 small_file

>Fix:

Changing the type of .ifid_ino to ino_t fixes the problem.

>Release-Note:

>Audit-Trail:
From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48799
Date: Sun, 11 May 2014 00:01:36 +0200

 --- sys/fs/cd9660/cd9660_vfsops.c.orig	2014-04-16 18:55:18.000000000 +0000
 +++ sys/fs/cd9660/cd9660_vfsops.c	2014-05-10 21:19:52.000000000 +0000
 @@ -641,7 +641,7 @@ cd9660_sync(struct mount *mp, int waitfo
  struct ifid {
  	ushort	ifid_len;
  	ushort	ifid_pad;
 -	int	ifid_ino;
 +	ino_t	ifid_ino;
  	long	ifid_start;
  };


From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48799
Date: Sun, 11 May 2014 12:17:19 +0200

 After finishing my assessment of struct ifid.ifid_start,
 i now understand better the remark of Taylor R Campbell in
   http://mail-index.netbsd.org/tech-kern/2014/05/10/msg017044.html
 "The ifid_start field seems to be unused, incidentally,
  so we needn't even expand the size of cd9660 file handles."

 I had the wrong impression that struct fid would be a maximum
 sized wrapper, like a union. But now i see the size inquiry
 protocol of VFS_VPTOFH(,fh_size).

 So i will update my patch by a few ifdefs which exclude the
 member .ifid_start from struct ifid by default.

 For completeness, i also quote Taylor's other remark:
 "As an aside, it seems from glancing at struct ufid in ufs/ufs/inode.h
  that UFS will have a similar 32-bit overflow problem."

From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48799
Date: Sun, 11 May 2014 12:22:32 +0200

 --- sys/fs/cd9660/cd9660_vfsops.c.orig	2014-04-16 18:55:18.000000000 +0000
 +++ sys/fs/cd9660/cd9660_vfsops.c	2014-05-11 10:02:49.000000000 +0000
 @@ -641,8 +641,12 @@ cd9660_sync(struct mount *mp, int waitfo
  struct ifid {
  	ushort	ifid_len;
  	ushort	ifid_pad;
 -	int	ifid_ino;
 +	ino_t	ifid_ino;
 +
 +#ifdef	ISOFS_DBG
  	long	ifid_start;
 +#endif
 +
  };

  /* ARGSUSED */
 @@ -792,6 +796,7 @@ cd9660_vget_internal(struct mount *mp, i
  		}

  #if 0
 +#ifdef	ISOFS_DBG
  		if (isonum_733(isodir->extent) +
  		    isonum_711(isodir->ext_attr_length) != ifhp->ifid_start) {
  			if (bp != 0)
 @@ -801,6 +806,7 @@ cd9660_vget_internal(struct mount *mp, i
  			    ifhp->ifid_start);
  			return (ESTALE);
  		}
 +#endif /* ISOFS_DBG */
  #endif
  	} else
  		bp = 0;
 @@ -914,7 +920,11 @@ cd9660_vptofh(struct vnode *vp, struct f
  	memset(&ifh, 0, sizeof(ifh));
  	ifh.ifid_len = sizeof(struct ifid);
  	ifh.ifid_ino = ip->i_number;
 +
 +#ifdef	ISOFS_DBG
  	ifh.ifid_start = ip->iso_start;
 +#endif
 +
  	memcpy(fhp, &ifh, sizeof(ifh));

  #ifdef	ISOFS_DBG

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48799 CVS commit: src/sys/fs/cd9660
Date: Tue, 13 May 2014 17:05:26 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue May 13 17:05:26 UTC 2014

 Modified Files:
 	src/sys/fs/cd9660: cd9660_vfsops.c

 Log Message:
 PR kern/48799: make filehandles properly use 64bit inodes on CD9660 file
 systems. Patch from Thomas Schmitt, with slight modifications.


 To generate a diff of this commit:
 cvs rdiff -u -r1.84 -r1.85 src/sys/fs/cd9660/cd9660_vfsops.c

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

State-Changed-From-To: open->closed
State-Changed-By: martin@NetBSD.org
State-Changed-When: Wed, 14 May 2014 06:33:09 +0000
State-Changed-Why:
Fixed, thanks for the patch!


>Unformatted:

Home	PR Database Search