r17125: Drastic problems require drastic solutions. There's

no way to get all the cases where kernel oplocks are
on and we can't open the file and get the correct
semantics (think about the open with truncate with
an attribute only open - we'd need a vfs change to
add the truncate(fname, len) call). So always drop
the share mode lock before doing any real fd opens and
then re-acquire it afterwards. We're already dealing
with the race in the create case, and we deal with
any other races in the same way. Volker, please
examine *carefully* :-). This should fix the problems
people reported with kernel oplocks being on.
Jeremy.
(This used to be commit 8171c4c404e9f382880c65daa0232f89e560f399)

2 files changed, 228 insertions, 109 deletions

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 7c04cdbe7c..b13b4f2a6c 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1403,6 +1403,8 @@ NTSTATUS open_file_ntcreate(connection_struct *conn,
 		}
 
 		if (!NT_STATUS_IS_OK(status)) {
+			uint32 can_access_mask;
+			BOOL can_access = True;
 
 			SMB_ASSERT(NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION));
 
@@ -1434,29 +1436,20 @@ NTSTATUS open_file_ntcreate(connection_struct *conn,
 			 * MS-Access. If a file open will fail due to share
 			 * permissions and also for security (access) reasons,
 			 * we need to return the access failed error, not the
-			 * share error. This means we must attempt to open the
-			 * file anyway in order to get the UNIX access error -
-			 * even if we're going to fail the open for share
-			 * reasons. This is bad, as we're burning another fd
-			 * if there are existing locks but there's nothing
-			 * else we can do. We also ensure we're not going to
-			 * create or tuncate the file as we only want an
-			 * access decision at this stage. JRA.
+			 * share error. We can't open the file due to kernel
+			 * oplock deadlock (it's possible we failed above on
+			 * the open_mode_check()) so use a userspace check.
 			 */
-			errno = 0;
-			fsp_open = open_file(fsp,conn,fname,psbuf,
-					     flags|(flags2&~(O_TRUNC|O_CREAT)),
-					     unx_mode,access_mask);
-
-			DEBUG(4,("open_file_ntcreate : share_mode deny - "
-				 "calling open_file with flags=0x%X "
-				 "flags2=0x%X mode=0%o returned %s\n",
-				 flags, (flags2&~(O_TRUNC|O_CREAT)),
-				 (unsigned int)unx_mode, nt_errstr(fsp_open)));
-
-			if (!NT_STATUS_IS_OK(fsp_open) && errno) {
-				/* Default error. */
-				status = NT_STATUS_ACCESS_DENIED;
+
+			if (flags & O_RDWR) {
+				can_access_mask = FILE_READ_DATA|FILE_WRITE_DATA;
+			} else {
+				can_access_mask = FILE_READ_DATA;
+			}
+
+			if (((flags & O_RDWR) && !CAN_WRITE(conn)) ||
+					!can_access_file(conn,fname,psbuf,can_access_mask)) {
+				can_access = False;
 			}
 
 			/* 
@@ -1503,13 +1496,14 @@ NTSTATUS open_file_ntcreate(connection_struct *conn,
 			}
 
 			TALLOC_FREE(lck);
-			if (NT_STATUS_IS_OK(fsp_open)) {
-				fd_close(conn, fsp);
+			if (can_access) {
 				/*
 				 * We have detected a sharing violation here
 				 * so return the correct error code
 				 */
 				status = NT_STATUS_SHARING_VIOLATION;
+			} else {
+				status = NT_STATUS_ACCESS_DENIED;
 			}
 			file_free(fsp);
 			return status;
@@ -1536,6 +1530,14 @@ NTSTATUS open_file_ntcreate(connection_struct *conn,
 		 (unsigned int)flags, (unsigned int)flags2,
 		 (unsigned int)unx_mode));
 
+	/* Drop the lock before doing any real file access. Allows kernel
+	   oplock breaks to be processed. Handle any races after the open
+	   call when we re-acquire the lock. */
+
+	if (lck) {
+		TALLOC_FREE(lck);
+	}
+
 	/*
 	 * open_file strips any O_TRUNC flags itself.
 	 */
@@ -1551,70 +1553,65 @@ NTSTATUS open_file_ntcreate(connection_struct *conn,
 		return fsp_open;
 	}
 
-	if (!file_existed) { 
-
-		/*
-		 * Deal with the race condition where two smbd's detect the
-		 * file doesn't exist and do the create at the same time. One
-		 * of them will win and set a share mode, the other (ie. this
-		 * one) should check if the requested share mode for this
-		 * create is allowed.
-		 */
-
-		/*
-		 * Now the file exists and fsp is successfully opened,
-		 * fsp->dev and fsp->inode are valid and should replace the
-		 * dev=0,inode=0 from a non existent file. Spotted by
-		 * Nadav Danieli <nadavd@exanet.com>. JRA.
-		 */
+	/*
+	 * Deal with the race condition where two smbd's detect the
+	 * file doesn't exist and do the create at the same time. One
+	 * of them will win and set a share mode, the other (ie. this
+	 * one) should check if the requested share mode for this
+	 * create is allowed.
+	 */
 
-		dev = fsp->dev;
-		inode = fsp->inode;
+	/*
+	 * Now the file exists and fsp is successfully opened,
+	 * fsp->dev and fsp->inode are valid and should replace the
+	 * dev=0,inode=0 from a non existent file. Spotted by
+	 * Nadav Danieli <nadavd@exanet.com>. JRA.
+	 */
 
-		lck = get_share_mode_lock(NULL, dev, inode,
-					conn->connectpath,
-					fname);
+	dev = fsp->dev;
+	inode = fsp->inode;
 
-		if (lck == NULL) {
-			DEBUG(0, ("open_file_ntcreate: Could not get share mode lock for %s\n", fname));
-			fd_close(conn, fsp);
-			file_free(fsp);
-			return NT_STATUS_SHARING_VIOLATION;
-		}
+	lck = get_share_mode_lock(NULL, dev, inode,
+				conn->connectpath,
+				fname);
 
-		status = open_mode_check(conn, fname, lck,
-					 access_mask, share_access,
-					 create_options, &file_existed);
+	if (lck == NULL) {
+		DEBUG(0, ("open_file_ntcreate: Could not get share mode lock for %s\n", fname));
+		fd_close(conn, fsp);
+		file_free(fsp);
+		return NT_STATUS_SHARING_VIOLATION;
+	}
 
-		if (!NT_STATUS_IS_OK(status)) {
-			struct deferred_open_record state;
+	status = open_mode_check(conn, fname, lck,
+				 access_mask, share_access,
+				 create_options, &file_existed);
 
-			fd_close(conn, fsp);
-			file_free(fsp);
+	if (!NT_STATUS_IS_OK(status)) {
+		struct deferred_open_record state;
 
-			state.delayed_for_oplocks = False;
-			state.dev = dev;
-			state.inode = inode;
+		fd_close(conn, fsp);
+		file_free(fsp);
 
-			/* Do it all over again immediately. In the second
-			 * round we will find that the file existed and handle
-			 * the DELETE_PENDING and FCB cases correctly. No need
-			 * to duplicate the code here. Essentially this is a
-			 * "goto top of this function", but don't tell
-			 * anybody... */
+		state.delayed_for_oplocks = False;
+		state.dev = dev;
+		state.inode = inode;
 
-			defer_open(lck, request_time, timeval_zero(),
-				   &state);
-			TALLOC_FREE(lck);
-			return status;
-		}
+		/* Do it all over again immediately. In the second
+		 * round we will find that the file existed and handle
+		 * the DELETE_PENDING and FCB cases correctly. No need
+		 * to duplicate the code here. Essentially this is a
+		 * "goto top of this function", but don't tell
+		 * anybody... */
 
-		/*
-		 * We exit this block with the share entry *locked*.....
-		 */
+		defer_open(lck, request_time, timeval_zero(),
+			   &state);
+		TALLOC_FREE(lck);
+		return status;
 	}
 
-	SMB_ASSERT(lck != NULL);
+	/*
+	 * The share entry is again *locked*.....
+	 */
 
 	/* note that we ignore failure for the following. It is
            basically a hack for NFS, and NFS will never set one of
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 73744cf26e..d265057ac0 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -3911,7 +3911,7 @@ BOOL set_unix_posix_acl(connection_struct *conn, files_struct *fsp, const char *
  Return -1 if no match, 0 if match and denied, 1 if match and allowed.
 ****************************************************************************/
 
-static int check_posix_acl_group_write(connection_struct *conn, const char *fname, SMB_STRUCT_STAT *psbuf)
+static int check_posix_acl_group_access(connection_struct *conn, const char *fname, SMB_STRUCT_STAT *psbuf, uint32 access_mask)
 {
 	SMB_ACL_T posix_acl = NULL;
 	int entry_id = SMB_ACL_FIRST_ENTRY;
@@ -3922,17 +3922,21 @@ static int check_posix_acl_group_write(connection_struct *conn, const char *fnam
 	int ret = -1;
 	gid_t cu_gid;
 
+	DEBUG(10,("check_posix_acl_group_access: requesting 0x%x on file %s\n",
+		(unsigned int)access_mask, fname ));
+
 	if ((posix_acl = SMB_VFS_SYS_ACL_GET_FILE(conn, fname, SMB_ACL_TYPE_ACCESS)) == NULL) {
 		goto check_stat;
 	}
 
-	/* First ensure the group mask allows group read. */
+	/* First ensure the group mask allows group access. */
 	/* Also check any user entries (these take preference over group). */
 
 	while ( SMB_VFS_SYS_ACL_GET_ENTRY(conn, posix_acl, entry_id, &entry) == 1) {
 		SMB_ACL_TAG_T tagtype;
 		SMB_ACL_PERMSET_T permset;
 		int have_write = -1;
+		int have_read = -1;
 
 		/* get_next... */
 		if (entry_id == SMB_ACL_FIRST_ENTRY)
@@ -3946,6 +3950,11 @@ static int check_posix_acl_group_write(connection_struct *conn, const char *fnam
 			goto check_stat;
 		}
 
+		have_read = SMB_VFS_SYS_ACL_GET_PERM(conn, permset, SMB_ACL_READ);
+		if (have_read == -1) {
+			goto check_stat;
+		}
+
 		have_write = SMB_VFS_SYS_ACL_GET_PERM(conn, permset, SMB_ACL_WRITE);
 		if (have_write == -1) {
 			goto check_stat;
@@ -3956,16 +3965,36 @@ static int check_posix_acl_group_write(connection_struct *conn, const char *fnam
 		 * canonicalize to 0 or 1.
 		 */	
 		have_write = (have_write ? 1 : 0);
+		have_read = (have_read ? 1 : 0);
 
 		switch(tagtype) {
 			case SMB_ACL_MASK:
 				seen_mask = True;
-				if (!have_write) {
-					/* We don't have any group or explicit user write permission. */
-					ret = -1; /* Allow caller to check "other" permissions. */
-					DEBUG(10,("check_posix_acl_group_write: file %s \
-refusing write due to mask.\n", fname));
-					goto done;
+				switch (access_mask) {
+					case FILE_READ_DATA:
+						if (!have_read) {
+							ret = -1;
+							DEBUG(10,("check_posix_acl_group_access: file %s "
+								"refusing read due to mask.\n", fname));
+							goto done;
+						}
+						break;
+					case FILE_WRITE_DATA:
+						if (!have_write) {
+							ret = -1;
+							DEBUG(10,("check_posix_acl_group_access: file %s "
+								"refusing write due to mask.\n", fname));
+							goto done;
+						}
+						break;
+					default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+						if (!have_write || !have_read) {
+							ret = -1;
+							DEBUG(10,("check_posix_acl_group_access: file %s "
+								"refusing read/write due to mask.\n", fname));
+							goto done;
+						}
+						break;
 				}
 				break;
 			case SMB_ACL_USER:
@@ -3977,9 +4006,21 @@ refusing write due to mask.\n", fname));
 				}
 				if (current_user.ut.uid == *puid) {
 					/* We have a uid match but we must ensure we have seen the acl mask. */
-					ret = have_write;
-					DEBUG(10,("check_posix_acl_group_write: file %s \
-match on user %u -> %s.\n", fname, (unsigned int)*puid, ret ? "can write" : "cannot write"));
+					switch (access_mask) {
+						case FILE_READ_DATA:
+							ret = have_read;
+							break;
+						case FILE_WRITE_DATA:
+							ret = have_write;
+							break;
+						default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+							ret = (have_write & have_read);
+							break;
+					}
+					DEBUG(10,("check_posix_acl_group_access: file %s "
+						"match on user %u -> %s.\n",
+						fname, (unsigned int)*puid,
+						ret ? "can access" : "cannot access"));
 					if (seen_mask) {
 						goto done;
 					}
@@ -4002,6 +4043,7 @@ match on user %u -> %s.\n", fname, (unsigned int)*puid, ret ? "can write" : "can
 		SMB_ACL_TAG_T tagtype;
 		SMB_ACL_PERMSET_T permset;
 		int have_write = -1;
+		int have_read = -1;
 
 		/* get_next... */
 		if (entry_id == SMB_ACL_FIRST_ENTRY)
@@ -4015,6 +4057,11 @@ match on user %u -> %s.\n", fname, (unsigned int)*puid, ret ? "can write" : "can
 			goto check_stat;
 		}
 
+		have_read = SMB_VFS_SYS_ACL_GET_PERM(conn, permset, SMB_ACL_READ);
+		if (have_read == -1) {
+			goto check_stat;
+		}
+
 		have_write = SMB_VFS_SYS_ACL_GET_PERM(conn, permset, SMB_ACL_WRITE);
 		if (have_write == -1) {
 			goto check_stat;
@@ -4025,6 +4072,7 @@ match on user %u -> %s.\n", fname, (unsigned int)*puid, ret ? "can write" : "can
 		 * canonicalize to 0 or 1.
 		 */	
 		have_write = (have_write ? 1 : 0);
+		have_read = (have_read ? 1 : 0);
 
 		switch(tagtype) {
 			case SMB_ACL_GROUP:
@@ -4049,13 +4097,25 @@ match on user %u -> %s.\n", fname, (unsigned int)*puid, ret ? "can write" : "can
 				for (cu_gid = get_current_user_gid_first(&i); cu_gid != (gid_t)-1;
 							cu_gid = get_current_user_gid_next(&i)) {
 					if (cu_gid == *pgid) {
-						ret = have_write;
-						DEBUG(10,("check_posix_acl_group_write: file %s \
-match on group %u -> can write.\n", fname, (unsigned int)cu_gid ));
+						switch (access_mask) {
+							case FILE_READ_DATA:
+								ret = have_read;
+								break;
+							case FILE_WRITE_DATA:
+								ret = have_write;
+								break;
+							default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+								ret = (have_write & have_read);
+								break;
+						}
 
-						/* If we don't have write permission this entry doesn't
+						DEBUG(10,("check_posix_acl_group_access: file %s "
+							"match on group %u -> can access.\n",
+							fname, (unsigned int)cu_gid ));
+
+						/* If we don't have access permission this entry doesn't
 							terminate the enumeration of the entries. */
-						if (have_write) {
+						if (ret) {
 							goto done;
 						}
 						/* But does terminate the group iteration. */
@@ -4072,12 +4132,12 @@ match on group %u -> can write.\n", fname, (unsigned int)cu_gid ));
 	/* If ret is -1 here we didn't match on the user entry or
 	   supplemental group entries. */
 	
-	DEBUG(10,("check_posix_acl_group_write: ret = %d before check_stat:\n", ret));
+	DEBUG(10,("check_posix_acl_group_access: ret = %d before check_stat:\n", ret));
 
   check_stat:
 
 	/*
-	 * We only check the S_IWGRP permissions if we haven't already
+	 * We only check the S_I[RW]GRP permissions if we haven't already
 	 * seen an owning group SMB_ACL_GROUP_OBJ ace entry. If there is an
 	 * SMB_ACL_GROUP_OBJ ace entry then the group bits in st_gid are
 	 * the same as the SMB_ACL_MASK bits, not the SMB_ACL_GROUP_OBJ
@@ -4094,16 +4154,33 @@ match on group %u -> can write.\n", fname, (unsigned int)cu_gid ));
 		for (cu_gid = get_current_user_gid_first(&i); cu_gid != (gid_t)-1;
 						cu_gid = get_current_user_gid_next(&i)) {
 			if (cu_gid == psbuf->st_gid) {
-				ret = (psbuf->st_mode & S_IWGRP) ? 1 : 0;
-				DEBUG(10,("check_posix_acl_group_write: file %s \
-match on owning group %u -> %s.\n", fname, (unsigned int)psbuf->st_gid, ret ? "can write" : "cannot write"));
+				switch (access_mask) {
+					case FILE_READ_DATA:
+						ret = (psbuf->st_mode & S_IRGRP) ? 1 : 0;
+						break;
+					case FILE_WRITE_DATA:
+						ret = (psbuf->st_mode & S_IWGRP) ? 1 : 0;
+						break;
+					default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+						if ((psbuf->st_mode & (S_IWGRP|S_IRGRP)) == (S_IWGRP|S_IRGRP)) {
+							ret = 1;
+						} else {
+							ret = 0;
+						}
+						break;
+				}
+				DEBUG(10,("check_posix_acl_group_access: file %s "
+					"match on owning group %u -> %s.\n",
+					fname, (unsigned int)psbuf->st_gid,
+					ret ? "can access" : "cannot access"));
 				break;
 			}
 		}
 
 		if (cu_gid == (gid_t)-1) {
-			DEBUG(10,("check_posix_acl_group_write: file %s \
-failed to match on user or group in token (ret = %d).\n", fname, ret ));
+			DEBUG(10,("check_posix_acl_group_access: file %s "
+				"failed to match on user or group in token (ret = %d).\n",
+				fname, ret ));
 		}
 	}
 
@@ -4113,7 +4190,7 @@ failed to match on user or group in token (ret = %d).\n", fname, ret ));
 		SMB_VFS_SYS_ACL_FREE_ACL(conn, posix_acl);
 	}
 
-	DEBUG(10,("check_posix_acl_group_write: file %s returning (ret = %d).\n", fname, ret ));
+	DEBUG(10,("check_posix_acl_group_access: file %s returning (ret = %d).\n", fname, ret ));
 	return ret;
 }
 
@@ -4169,7 +4246,7 @@ BOOL can_delete_file_in_directory(connection_struct *conn, const char *fname)
 #endif
 
 	/* Check group or explicit user acl entry write access. */
-	ret = check_posix_acl_group_write(conn, dname, &sbuf);
+	ret = check_posix_acl_group_access(conn, dname, &sbuf, FILE_WRITE_DATA);
 	if (ret == 0 || ret == 1) {
 		return ret ? True : False;
 	}
@@ -4179,15 +4256,23 @@ BOOL can_delete_file_in_directory(connection_struct *conn, const char *fname)
 }
 
 /****************************************************************************
- Actually emulate the in-kernel access checking for write access. We need
+ Actually emulate the in-kernel access checking for read/write access. We need
  this to successfully check for ability to write for dos filetimes.
  Note this doesn't take into account share write permissions.
 ****************************************************************************/
 
-BOOL can_write_to_file(connection_struct *conn, const char *fname, SMB_STRUCT_STAT *psbuf)
+BOOL can_access_file(connection_struct *conn, const char *fname, SMB_STRUCT_STAT *psbuf, uint32 access_mask)
 {
 	int ret;
 
+	if (!(access_mask & (FILE_READ_DATA|FILE_WRITE_DATA))) {
+		return False;
+	}
+	access_mask &= (FILE_READ_DATA|FILE_WRITE_DATA);
+
+	DEBUG(10,("can_access_file: requesting 0x%x on file %s\n",
+		(unsigned int)access_mask, fname ));
+
 	if (current_user.ut.uid == 0 || conn->admin_user) {
 		/* I'm sorry sir, I didn't know you were root... */
 		return True;
@@ -4200,19 +4285,56 @@ BOOL can_write_to_file(connection_struct *conn, const char *fname, SMB_STRUCT_ST
 		}
 	}
 
-	/* Check primary owner write access. */
+	/* Check primary owner access. */
 	if (current_user.ut.uid == psbuf->st_uid) {
-		return (psbuf->st_mode & S_IWUSR) ? True : False;
+		switch (access_mask) {
+			case FILE_READ_DATA:
+				return (psbuf->st_mode & S_IRUSR) ? True : False;
+
+			case FILE_WRITE_DATA:
+				return (psbuf->st_mode & S_IWUSR) ? True : False;
+
+			default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+
+				if ((psbuf->st_mode & (S_IWUSR|S_IRUSR)) == (S_IWUSR|S_IRUSR)) {
+					return True;
+				} else {
+					return False;
+				}
+		}
 	}
 
-	/* Check group or explicit user acl entry write access. */
-	ret = check_posix_acl_group_write(conn, fname, psbuf);
+	/* Check group or explicit user acl entry access. */
+	ret = check_posix_acl_group_access(conn, fname, psbuf, access_mask);
 	if (ret == 0 || ret == 1) {
 		return ret ? True : False;
 	}
 
-	/* Finally check other write access. */
-	return (psbuf->st_mode & S_IWOTH) ? True : False;
+	/* Finally check other access. */
+	switch (access_mask) {
+		case FILE_READ_DATA:
+			return (psbuf->st_mode & S_IROTH) ? True : False;
+
+		case FILE_WRITE_DATA:
+			return (psbuf->st_mode & S_IWOTH) ? True : False;
+
+		default: /* FILE_READ_DATA|FILE_WRITE_DATA */
+
+			if ((psbuf->st_mode & (S_IWOTH|S_IROTH)) == (S_IWOTH|S_IROTH)) {
+				return True;
+			}
+	}
+	return False;
+}
+
+/****************************************************************************
+ Userspace check for write access.
+ Note this doesn't take into account share write permissions.
+****************************************************************************/
+
+BOOL can_write_to_file(connection_struct *conn, const char *fname, SMB_STRUCT_STAT *psbuf)
+{
+	return can_access_file(conn, fname, psbuf, FILE_WRITE_DATA);
 }
 
 /********************************************************************