Bug 151082 - [zfs] [patch] sappend-flaged files on ZFS not working correctly
Summary: [zfs] [patch] sappend-flaged files on ZFS not working correctly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-fs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-29 18:50 UTC by cal
Modified: 2010-11-28 10:16 UTC (History)
0 users

See Also:

Attachments
file.diff (739 bytes, patch)
2010-09-29 18:50 UTC, cal 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description cal 2010-09-29 18:50:00 UTC 
Background: When moving from pre-8.0 to FreeBSD 8.1 on ZFS I 
realised that the bash history is no longer beeing written if the 
file has the sappend flag set. Other programs like script (1) 
however are surprisingly still able to write to such files. So it 
seemed that they handle file IO differently.

(Looking at the source code of both I learned that bash uses write 
(2) while script uses fwrite (3)... After some more testing and 
investigation,) it seems that zfs behaves differently to ufs in one 
point.

If I open (2) a file with O_APPEND the fd position pointer is set 
to the start of the file. When I then write to an fd on a ufs FS in 
FB6.4, it is automatically moved to the end and the bytes are 
appended. No problems appear with write to the sappend-flages file.

However if I do the same on FB8.1/ZFS, the write fails as "not 
permitted". When I manually move the position pointer to the end of 
the file, it works as axpected (see test case below).

See also the thread on the freebsd-fs mailinglist: http://lists.freebsd.org/pipermail/freebsd-fs/2010-September/thread.html#9528

Fix: by Markus Gebert, original (and follow ups): http://lists.freebsd.org/pipermail/freebsd-fs/2010-September/009529.html
-------------------------------
To me, it seems that the zfs_write() VOP incorrectly uses FAPPEND instead of IO_APPEND when checking the ioflag argument to see if the current write is an appending one, here:

----
        /*
         * If immutable or not appending then return EPERM
         */
        pflags = zp->z_phys->zp_flags;
        if ((pflags & (ZFS_IMMUTABLE | ZFS_READONLY)) ||
            ((pflags & ZFS_APPENDONLY) && !(ioflag & FAPPEND) &&
            (uio->uio_loffset < zp->z_phys->zp_size))) {
                ZFS_EXIT(zfsvfs);
                return (EPERM);
        }
----

The function comment only mentions IO_APPEND and even later on in zfs_write() IO_APPEND is used to check wether the offset should be changed to EOF, so FAPPEND really seems to wrong in the above permission check.

CURRENT and STABLE-8 seem to be affected to. The following patch seems to fix it (at least Michi's test case works fine with it):

----
How-To-Repeat: FreeBSD 6.4/UFS: OK

root@fb64 [~/michi]# touch gaga
root@fb64 [~/michi]# chflags sappend gaga 
root@fb64 [~/michi]# ./write 
fd: 3
pos: 0
bytes written: 6
pos: 50
bytes written: 6

FreeBSD 8.1/ZFS: BROKEN

root@fb81 [~/michi]# touch gaga
root@fb81 [~/michi]# chflags sappend gaga 
root@fb81 [~/michi]# ./write
fd: 3
pos: 0
ERROR: Operation not permitted
pos: 147
bytes written: 6

The test source code:

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>

int main(void){
        int file= open("gaga",O_WRONLY|O_APPEND|O_CREAT,0666);
        int bytes_written= 0;
        printf("fd: %i\n",file);
        printf("pos: %i\n",lseek(file,0,SEEK_CUR));
        if ((bytes_written= write(file,"write\n",6)) == -1){
                printf("ERROR: %s\n",strerror(errno));
        }else{
                printf("bytes written: %i\n",bytes_written);
        }
        lseek(file,0,SEEK_END);
        printf("pos: %i\n",lseek(file,0,SEEK_CUR));
        if ((bytes_written= write(file,"write\n",6)) == -1){
                printf("ERROR: %s\n",strerror(errno));
        }else{
                printf("bytes written: %i\n",bytes_written);
        }
        close(file);
}
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2010-10-01 01:48:32 UTC 
Responsible Changed
From-To: freebsd-bugs->freebsd-fs

Over to maintainer(s).
Comment 2 dfilter service freebsd_committer freebsd_triage 2010-10-09 00:01:44 UTC 
Author: mm
Date: Fri Oct  8 23:01:38 2010
New Revision: 213634
URL: http://svn.freebsd.org/changeset/base/213634

Log:
  Change FAPPEND to IO_APPEND as this is a ioflag and not a fflag.
  This corrects writing to append-only files on ZFS.
  
  PR:		kern/149495 [1], kern/151082 [2]
  Submitted by:	Daniel Zhelev <daniel@zhelev.biz> [1], Michael Naef <cal@linu.gs> [2]
  Approved by:	delphij (mentor)
  MFC after:	1 week

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Fri Oct  8 21:54:33 2010	(r213633)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Fri Oct  8 23:01:38 2010	(r213634)
@@ -755,7 +755,7 @@ zfs_write(vnode_t *vp, uio_t *uio, int i
 	 */
 	pflags = zp->z_phys->zp_flags;
 	if ((pflags & (ZFS_IMMUTABLE | ZFS_READONLY)) ||
-	    ((pflags & ZFS_APPENDONLY) && !(ioflag & FAPPEND) &&
+	    ((pflags & ZFS_APPENDONLY) && !(ioflag & IO_APPEND) &&
 	    (uio->uio_loffset < zp->z_phys->zp_size))) {
 		ZFS_EXIT(zfsvfs);
 		return (EPERM);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 3 Jaakko Heinonen freebsd_committer freebsd_triage 2010-11-28 09:22:58 UTC 
State Changed
From-To: open->patched

Patched in head (r213634).
Comment 4 dfilter service freebsd_committer freebsd_triage 2010-11-28 09:52:15 UTC 
Author: mm
Date: Sun Nov 28 09:52:06 2010
New Revision: 215992
URL: http://svn.freebsd.org/changeset/base/215992

Log:
  MFC r213634, r213673:
  
  MFC r213634:
  Change FAPPEND to IO_APPEND as this is a ioflag and not a fflag.
  This corrects writing to append-only files on ZFS.
  
  MFC r213673 (pjd):
  Provide internal ioflags() function that converts ioflag provided by FreeBSD's
  VFS to OpenSolaris-specific ioflag expected by ZFS. Use it for read and write
  operations.
  
  PR:		kern/149495 [1], kern/151082 [2]
  Submitted by:	Daniel Zhelev <daniel@zhelev.biz> [1], Michael Naef <cal@linu.gs> [2]
  Reviewed by:	mm (r213673)
  Approved by:	delphij (mentor)

Modified:
  stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
==============================================================================
--- stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Sun Nov 28 09:35:56 2010	(r215991)
+++ stable/8/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Sun Nov 28 09:52:06 2010	(r215992)
@@ -682,7 +682,7 @@ zfs_prefault_write(ssize_t n, struct uio
  *	IN:	vp	- vnode of file to be written to.
  *		uio	- structure supplying write location, range info,
  *			  and data buffer.
- *		ioflag	- IO_APPEND flag set if in append mode.
+ *		ioflag	- FAPPEND flag set if in append mode.
  *		cr	- credentials of caller.
  *		ct	- caller context (NFS/CIFS fem monitor only)
  *
@@ -749,7 +749,7 @@ zfs_write(vnode_t *vp, uio_t *uio, int i
 	/*
 	 * If in append mode, set the io offset pointer to eof.
 	 */
-	if (ioflag & IO_APPEND) {
+	if (ioflag & FAPPEND) {
 		/*
 		 * Range lock for a file append:
 		 * The value for the start of range will be determined by
@@ -4176,6 +4176,21 @@ zfs_setsecattr(vnode_t *vp, vsecattr_t *
 }
 
 static int
+ioflags(int ioflags)
+{
+	int flags = 0;
+
+	if (ioflags & IO_APPEND)
+		flags |= FAPPEND;
+	if (ioflags & IO_NDELAY)
+        	flags |= FNONBLOCK;
+	if (ioflags & IO_SYNC)
+		flags |= (FSYNC | FDSYNC | FRSYNC);
+
+	return (flags);
+}
+
+static int
 zfs_getpages(struct vnode *vp, vm_page_t *m, int count, int reqpage)
 {
 	znode_t *zp = VTOZ(vp);
@@ -4322,7 +4337,8 @@ zfs_freebsd_read(ap)
 	} */ *ap;
 {
 
-	return (zfs_read(ap->a_vp, ap->a_uio, ap->a_ioflag, ap->a_cred, NULL));
+	return (zfs_read(ap->a_vp, ap->a_uio, ioflags(ap->a_ioflag),
+	    ap->a_cred, NULL));
 }
 
 static int
@@ -4335,7 +4351,8 @@ zfs_freebsd_write(ap)
 	} */ *ap;
 {
 
-	return (zfs_write(ap->a_vp, ap->a_uio, ap->a_ioflag, ap->a_cred, NULL));
+	return (zfs_write(ap->a_vp, ap->a_uio, ioflags(ap->a_ioflag),
+	    ap->a_cred, NULL));
 }
 
 static int
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 dfilter service freebsd_committer freebsd_triage 2010-11-28 10:05:34 UTC 
Author: mm
Date: Sun Nov 28 10:05:26 2010
New Revision: 215993
URL: http://svn.freebsd.org/changeset/base/215993

Log:
  MFC r213634, r213673:
  
  MFC r213634:
  Change FAPPEND to IO_APPEND as this is a ioflag and not a fflag.
  This corrects writing to append-only files on ZFS.
  
  MFC r213673 (pjd):
  Provide internal ioflags() function that converts ioflag provided by FreeBSD's
  VFS to OpenSolaris-specific ioflag expected by ZFS. Use it for read and write
  operations.
  
  PR:		kern/149495 [1], kern/151082 [2]
  Submitted by:	Daniel Zhelev <daniel@zhelev.biz> [1], Michael Naef <cal@linu.gs> [2]
  Reviewed by:	mm (r213673)
  Approved by:	delphij (mentor)

Modified:
  stable/7/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
Directory Properties:
  stable/7/sys/   (props changed)
  stable/7/sys/cddl/contrib/opensolaris/   (props changed)
  stable/7/sys/contrib/dev/acpica/   (props changed)
  stable/7/sys/contrib/pf/   (props changed)

Modified: stable/7/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
==============================================================================
--- stable/7/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Sun Nov 28 09:52:06 2010	(r215992)
+++ stable/7/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c	Sun Nov 28 10:05:26 2010	(r215993)
@@ -660,7 +660,7 @@ zfs_prefault_write(ssize_t n, struct uio
  *	IN:	vp	- vnode of file to be written to.
  *		uio	- structure supplying write location, range info,
  *			  and data buffer.
- *		ioflag	- IO_APPEND flag set if in append mode.
+ *		ioflag	- FAPPEND flag set if in append mode.
  *		cr	- credentials of caller.
  *		ct	- caller context (NFS/CIFS fem monitor only)
  *
@@ -726,7 +726,7 @@ zfs_write(vnode_t *vp, uio_t *uio, int i
 	/*
 	 * If in append mode, set the io offset pointer to eof.
 	 */
-	if (ioflag & IO_APPEND) {
+	if (ioflag & FAPPEND) {
 		/*
 		 * Range lock for a file append:
 		 * The value for the start of range will be determined by
@@ -3887,6 +3887,21 @@ zfs_setsecattr(vnode_t *vp, vsecattr_t *
 #endif	/* TODO */
 
 static int
+ioflags(int ioflags)
+{
+	int flags = 0;
+
+	if (ioflags & IO_APPEND)
+		flags |= FAPPEND;
+	if (ioflags & IO_NDELAY)
+        	flags |= FNONBLOCK;
+	if (ioflags & IO_SYNC)
+		flags |= (FSYNC | FDSYNC | FRSYNC);
+
+	return (flags);
+}
+
+static int
 zfs_freebsd_open(ap)
 	struct vop_open_args /* {
 		struct vnode *a_vp;
@@ -3944,7 +3959,8 @@ zfs_freebsd_read(ap)
 	} */ *ap;
 {
 
-	return (zfs_read(ap->a_vp, ap->a_uio, ap->a_ioflag, ap->a_cred, NULL));
+	return (zfs_read(ap->a_vp, ap->a_uio, ioflags(ap->a_ioflag),
+	    ap->a_cred, NULL));
 }
 
 static int
@@ -3957,7 +3973,8 @@ zfs_freebsd_write(ap)
 	} */ *ap;
 {
 
-	return (zfs_write(ap->a_vp, ap->a_uio, ap->a_ioflag, ap->a_cred, NULL));
+	return (zfs_write(ap->a_vp, ap->a_uio, ioflags(ap->a_ioflag),
+	    ap->a_cred, NULL));
 }
 
 static int
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 6 Martin Matuska freebsd_committer freebsd_triage 2010-11-28 10:16:54 UTC 
State Changed
From-To: patched->closed

Resolved. Thanks!