[PATCH 0/4] cifs: CONFIG_CIFS_EXPERIMENTAL removal (try #2)

[PATCH 0/4] cifs: CONFIG_CIFS_EXPERIMENTAL removal (try #2)

[Jeff Layton]

This is the second version of this patchset. The changes since the last
set are:

1) The patch to remove "/proc/fs/cifs/Experimental" did not remove the
deregistration of that file, which caused a warning on rmmod.

2) The patch to remove "/proc/fs/cifs/Experimental" now adds a new
module parameter so that people relying on it to allow zero-copy
writes with signing have a way to continue using that.

A modified description of the set follows...

The CONFIG_CIFS_EXPERIMENTAL KConfig option is the sort of thing that
gives distro packagers nightmares. The things that live under it are
impossible to predict for someone who isn't following development
upstream.

We usually want bleeding-edge distros like Fedora to turn on these sorts
of bleeding edge features. But, it's difficult for them to do so with any
confidence because so much of the code under this option is just plain
broken. If we have code that needs to be conditionally compiled in,
then it generally ought to be given its own KConfig option.

This patchset eliminates the CONFIG_CIFS_EXPERIMENTAL Kconfig option.
Code that currently resides under this option is either moved to being
built in by default or is removed from the kernel altogether.

The last patch in the series also removes /proc/fs/cifs/Experimental 
and replaces with with a bool module_parm that better describes what
it does.

I've tested this by building cifs.ko with a variety of different Kconfig
combinations and it seems to be OK. I've also played with the new
module option and it seems to work properly.

I think this patchset is appropriate for 2.6.38, though I won't object
if you want to merge it sooner.

Jeff Layton (4):
  cifs: remove export_ops code
  cifs: move "ntlmssp" and "local_leases" options out of experimental
    code
  cifs: remove CIFSSMBQueryReparseLinkInfo and CONFIG_CIFS_EXPERIMENTAL
  cifs: replace /proc/fs/cifs/Experimental with a module parm

 fs/cifs/Kconfig      |   13 ------
 fs/cifs/Makefile     |    2 +-
 fs/cifs/README       |   28 -------------
 fs/cifs/cifs_debug.c |   43 --------------------
 fs/cifs/cifsfs.c     |   13 ++----
 fs/cifs/cifsfs.h     |    4 --
 fs/cifs/cifsglob.h   |    2 +-
 fs/cifs/cifsproto.h  |    6 ---
 fs/cifs/cifssmb.c    |  109 +-------------------------------------------------
 fs/cifs/connect.c    |    4 --
 fs/cifs/export.c     |   67 ------------------------------
 fs/cifs/file.c       |    4 +-
 fs/cifs/sess.c       |  103 +++++++++++++++++++++-------------------------
 13 files changed, 56 insertions(+), 342 deletions(-)
 delete mode 100644 fs/cifs/export.c

-- 
1.7.3.2

[PATCH 1/4] cifs: remove export_ops code

[Jeff Layton]

Stubs for this code were added under the CONFIG_CIFS_EXPERIMENTAL tag in
2007, but were never completed. The upshot -- anyone who turns on
CONFIG_CIFS_EXPERIMENTAL gets a filesystem that pretends to allow exporting
via nfsd. Trying to actually use it will just get you an error once
nfsd tries to walk into the directory.

Remove this code since it's unfinished. We can reintroduce it once someone
spends time on it and makes it usable.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/Makefile |    2 +-
 fs/cifs/cifsfs.c |    7 -----
 fs/cifs/cifsfs.h |    4 ---
 fs/cifs/export.c |   67 ------------------------------------------------------
 4 files changed, 1 insertions(+), 79 deletions(-)
 delete mode 100644 fs/cifs/export.c

diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
index 43b19dd..ee8a33c 100644
--- a/fs/cifs/Makefile
+++ b/fs/cifs/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_CIFS) += cifs.o
 cifs-y := cifsfs.o cifssmb.o cifs_debug.o connect.o dir.o file.o inode.o \
 	  link.o misc.o netmisc.o smbdes.o smbencrypt.o transport.o asn1.o \
 	  md4.o md5.o cifs_unicode.o nterr.o xattr.o cifsencrypt.o \
-	  readdir.o ioctl.o sess.o export.o
+	  readdir.o ioctl.o sess.o
 
 cifs-$(CONFIG_CIFS_ACL) += cifsacl.o
 
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index db28c28..48582b7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -174,13 +174,6 @@ cifs_read_super(struct super_block *sb, void *data,
 		goto out_no_root;
 	}
 
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
-		cFYI(1, "export ops supported");
-		sb->s_export_op = &cifs_export_ops;
-	}
-#endif /* EXPERIMENTAL */
-
 	return 0;
 
 out_no_root:
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 897b2b2..1d6d021 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -108,9 +108,5 @@ extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-extern const struct export_operations cifs_export_ops;
-#endif /* EXPERIMENTAL */
-
 #define CIFS_VERSION   "1.68"
 #endif				/* _CIFSFS_H */
diff --git a/fs/cifs/export.c b/fs/cifs/export.c
deleted file mode 100644
index 993f820..0000000
--- a/fs/cifs/export.c
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- *   fs/cifs/export.c
- *
- *   Copyright (C) International Business Machines  Corp., 2007
- *   Author(s): Steve French (sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org)
- *
- *   Common Internet FileSystem (CIFS) client
- *
- *   Operations related to support for exporting files via NFSD
- *
- *   This library is free software; you can redistribute it and/or modify
- *   it under the terms of the GNU Lesser General Public License as published
- *   by the Free Software Foundation; either version 2.1 of the License, or
- *   (at your option) any later version.
- *
- *   This library is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY; without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU Lesser General Public License for more details.
- *
- *   You should have received a copy of the GNU Lesser General Public License
- *   along with this library; if not, write to the Free Software
- *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- */
-
- /*
-  * See Documentation/filesystems/nfs/Exporting
-  * and examples in fs/exportfs
-  *
-  * Since cifs is a network file system, an "fsid" must be included for
-  * any nfs exports file entries which refer to cifs paths.  In addition
-  * the cifs mount must be mounted with the "serverino" option (ie use stable
-  * server inode numbers instead of locally generated temporary ones).
-  * Although cifs inodes do not use generation numbers (have generation number
-  * of zero) - the inode number alone should be good enough for simple cases
-  * in which users want to export cifs shares with NFS. The decode and encode
-  * could be improved by using a new routine which expects 64 bit inode numbers
-  * instead of the default 32 bit routines in fs/exportfs
-  *
-  */
-
-#include <linux/fs.h>
-#include <linux/exportfs.h>
-#include "cifsglob.h"
-#include "cifs_debug.h"
-#include "cifsfs.h"
-
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-static struct dentry *cifs_get_parent(struct dentry *dentry)
-{
-	/* BB need to add code here eventually to enable export via NFSD */
-	cFYI(1, "get parent for %p", dentry);
-	return ERR_PTR(-EACCES);
-}
-
-const struct export_operations cifs_export_ops = {
-	.get_parent = cifs_get_parent,
-/*	Following five export operations are unneeded so far and can default:
-	.get_dentry =
-	.get_name =
-	.find_exported_dentry =
-	.decode_fh =
-	.encode_fs =  */
-};
-
-#endif /* EXPERIMENTAL */
-
-- 
1.7.3.2

[PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Jeff Layton]

I see no real need to leave these sorts of options under an
EXPERIMENTAL ifdef. Since you need a mount option to turn this code
on, that only blows out the testing matrix.

local_leases has been under the EXPERIMENTAL tag for some time, but
it's only the mount option that's under this label. Move it out
from under this tag.

The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
option to turn it on, and in the future any distro will reasonably
want this enabled. Go ahead and move it out from under the
EXPERIMENTAL tag.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifssmb.c |    5 +--
 fs/cifs/connect.c |    4 --
 fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
 3 files changed, 48 insertions(+), 64 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index d7957a3..4df1d10 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -401,15 +401,12 @@ CIFSSMBNegotiate(unsigned int xid, struct cifsSesInfo *ses)
 	else if ((secFlags & CIFSSEC_AUTH_MASK) == CIFSSEC_MAY_KRB5) {
 		cFYI(1, "Kerberos only mechanism, enable extended security");
 		pSMB->hdr.Flags2 |= SMBFLG2_EXT_SEC;
-	}
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-	else if ((secFlags & CIFSSEC_MUST_NTLMSSP) == CIFSSEC_MUST_NTLMSSP)
+	} else if ((secFlags & CIFSSEC_MUST_NTLMSSP) == CIFSSEC_MUST_NTLMSSP)
 		pSMB->hdr.Flags2 |= SMBFLG2_EXT_SEC;
 	else if ((secFlags & CIFSSEC_AUTH_MASK) == CIFSSEC_MAY_NTLMSSP) {
 		cFYI(1, "NTLMSSP only mechanism, enable extended security");
 		pSMB->hdr.Flags2 |= SMBFLG2_EXT_SEC;
 	}
-#endif
 
 	count = 0;
 	for (i = 0; i < CIFS_NUM_PROT; i++) {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 23098c2..9fbe7c5 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -985,13 +985,11 @@ cifs_parse_mount_options(char *options, const char *devname,
 				return 1;
 			} else if (strnicmp(value, "krb5", 4) == 0) {
 				vol->secFlg |= CIFSSEC_MAY_KRB5;
-#ifdef CONFIG_CIFS_EXPERIMENTAL
 			} else if (strnicmp(value, "ntlmsspi", 8) == 0) {
 				vol->secFlg |= CIFSSEC_MAY_NTLMSSP |
 					CIFSSEC_MUST_SIGN;
 			} else if (strnicmp(value, "ntlmssp", 7) == 0) {
 				vol->secFlg |= CIFSSEC_MAY_NTLMSSP;
-#endif
 			} else if (strnicmp(value, "ntlmv2i", 7) == 0) {
 				vol->secFlg |= CIFSSEC_MAY_NTLMV2 |
 					CIFSSEC_MUST_SIGN;
@@ -1342,10 +1340,8 @@ cifs_parse_mount_options(char *options, const char *devname,
 			vol->no_psx_acl = 0;
 		} else if (strnicmp(data, "noacl", 5) == 0) {
 			vol->no_psx_acl = 1;
-#ifdef CONFIG_CIFS_EXPERIMENTAL
 		} else if (strnicmp(data, "locallease", 6) == 0) {
 			vol->local_lease = 1;
-#endif
 		} else if (strnicmp(data, "sign", 4) == 0) {
 			vol->secFlg |= CIFSSEC_MUST_SIGN;
 		} else if (strnicmp(data, "seal", 4) == 0) {
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 7b01d3f..2997533 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -420,7 +420,6 @@ static int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len,
 	return 0;
 }
 
-#ifdef CONFIG_CIFS_EXPERIMENTAL
 /* BB Move to ntlmssp.c eventually */
 
 /* We do not malloc the blob, it is passed in pbuffer, because
@@ -572,7 +571,6 @@ static void setup_ntlmssp_neg_req(SESSION_SETUP_ANDX *pSMB,
 
 	return;
 }
-#endif
 
 int
 CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
@@ -814,71 +812,64 @@ ssetup_ntlmssp_authenticate:
 		rc = -ENOSYS;
 		goto ssetup_exit;
 #endif /* CONFIG_CIFS_UPCALL */
-	} else {
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-		if (type == RawNTLMSSP) {
-			if ((pSMB->req.hdr.Flags2 & SMBFLG2_UNICODE) == 0) {
-				cERROR(1, "NTLMSSP requires Unicode support");
-				rc = -ENOSYS;
+	} else if (type == RawNTLMSSP) {
+		if ((pSMB->req.hdr.Flags2 & SMBFLG2_UNICODE) == 0) {
+			cERROR(1, "NTLMSSP requires Unicode support");
+			rc = -ENOSYS;
+			goto ssetup_exit;
+		}
+
+		cFYI(1, "ntlmssp session setup phase %d", phase);
+		pSMB->req.hdr.Flags2 |= SMBFLG2_EXT_SEC;
+		capabilities |= CAP_EXTENDED_SECURITY;
+		pSMB->req.Capabilities |= cpu_to_le32(capabilities);
+		if (phase == NtLmNegotiate) {
+			setup_ntlmssp_neg_req(pSMB, ses);
+			iov[1].iov_len = sizeof(NEGOTIATE_MESSAGE);
+			iov[1].iov_base = &pSMB->req.SecurityBlob[0];
+		} else if (phase == NtLmAuthenticate) {
+			/* 5 is an empirical value, large enought to
+			 * hold authenticate message, max 10 of
+			 * av paris, doamin,user,workstation mames,
+			 * flags etc..
+			 */
+			ntlmsspblob = kmalloc(
+				5*sizeof(struct _AUTHENTICATE_MESSAGE),
+				GFP_KERNEL);
+			if (!ntlmsspblob) {
+				cERROR(1, "Can't allocate NTLMSSP");
+				rc = -ENOMEM;
 				goto ssetup_exit;
 			}
 
-			cFYI(1, "ntlmssp session setup phase %d", phase);
-			pSMB->req.hdr.Flags2 |= SMBFLG2_EXT_SEC;
-			capabilities |= CAP_EXTENDED_SECURITY;
-			pSMB->req.Capabilities |= cpu_to_le32(capabilities);
-			if (phase == NtLmNegotiate) {
-				setup_ntlmssp_neg_req(pSMB, ses);
-				iov[1].iov_len = sizeof(NEGOTIATE_MESSAGE);
-				iov[1].iov_base = &pSMB->req.SecurityBlob[0];
-			} else if (phase == NtLmAuthenticate) {
-				/* 5 is an empirical value, large enought to
-				 * hold authenticate message, max 10 of
-				 * av paris, doamin,user,workstation mames,
-				 * flags etc..
-				 */
-				ntlmsspblob = kmalloc(
-					5*sizeof(struct _AUTHENTICATE_MESSAGE),
-					GFP_KERNEL);
-				if (!ntlmsspblob) {
-					cERROR(1, "Can't allocate NTLMSSP");
-					rc = -ENOMEM;
-					goto ssetup_exit;
-				}
-
-				rc = build_ntlmssp_auth_blob(ntlmsspblob,
-							&blob_len, ses, nls_cp);
-				if (rc)
-					goto ssetup_exit;
-				iov[1].iov_len = blob_len;
-				iov[1].iov_base = ntlmsspblob;
-				pSMB->req.SecurityBlobLength =
-					cpu_to_le16(blob_len);
-				/* Make sure that we tell the server that we
-				   are using the uid that it just gave us back
-				   on the response (challenge) */
-				smb_buf->Uid = ses->Suid;
-			} else {
-				cERROR(1, "invalid phase %d", phase);
-				rc = -ENOSYS;
+			rc = build_ntlmssp_auth_blob(ntlmsspblob,
+						&blob_len, ses, nls_cp);
+			if (rc)
 				goto ssetup_exit;
-			}
-			/* unicode strings must be word aligned */
-			if ((iov[0].iov_len + iov[1].iov_len) % 2) {
-				*bcc_ptr = 0;
-				bcc_ptr++;
-			}
-			unicode_oslm_strings(&bcc_ptr, nls_cp);
+			iov[1].iov_len = blob_len;
+			iov[1].iov_base = ntlmsspblob;
+			pSMB->req.SecurityBlobLength = cpu_to_le16(blob_len);
+			/* Make sure that we tell the server that we
+			   are using the uid that it just gave us back
+			   on the response (challenge) */
+			smb_buf->Uid = ses->Suid;
 		} else {
-			cERROR(1, "secType %d not supported!", type);
+			cERROR(1, "invalid phase %d", phase);
 			rc = -ENOSYS;
 			goto ssetup_exit;
 		}
-#else
+
+		/* unicode strings must be word aligned */
+		if ((iov[0].iov_len + iov[1].iov_len) % 2) {
+			*bcc_ptr = 0;
+			bcc_ptr++;
+		}
+
+		unicode_oslm_strings(&bcc_ptr, nls_cp);
+	} else {
 		cERROR(1, "secType %d not supported!", type);
 		rc = -ENOSYS;
 		goto ssetup_exit;
-#endif
 	}
 
 	iov[2].iov_base = str_area;
-- 
1.7.3.2

[PATCH 3/4] cifs: remove CIFSSMBQueryReparseLinkInfo and CONFIG_CIFS_EXPERIMENTAL

[Jeff Layton]

Nothing calls this function. Remove it.

With this, the last user of CONFIG_CIFS_EXPERIMENTAL has been
removed. Also, update the README.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/Kconfig     |   13 ------
 fs/cifs/README      |   12 ------
 fs/cifs/cifsproto.h |    6 ---
 fs/cifs/cifssmb.c   |  104 ---------------------------------------------------
 4 files changed, 0 insertions(+), 135 deletions(-)

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index ee45648..27917a5 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -151,16 +151,3 @@ config CIFS_ACL
 	    Allows to fetch CIFS/NTFS ACL from the server.  The DACL blob
 	    is handed over to the application/caller.
 
-config CIFS_EXPERIMENTAL
-	  bool "CIFS Experimental Features (EXPERIMENTAL)"
-	  depends on CIFS && EXPERIMENTAL
-	  help
-	    Enables cifs features under testing. These features are
-	    experimental and currently include DFS support and directory
-	    change notification ie fcntl(F_DNOTIFY), as well as the upcall
-	    mechanism which will be used for Kerberos session negotiation
-	    and uid remapping.  Some of these features also may depend on
-	    setting a value of 1 to the pseudo-file /proc/fs/cifs/Experimental
-	    (which is disabled by default). See the file fs/cifs/README
-	    for more details.  If unsure, say N.
-
diff --git a/fs/cifs/README b/fs/cifs/README
index 46af99a..60a8ba5 100644
--- a/fs/cifs/README
+++ b/fs/cifs/README
@@ -715,18 +715,6 @@ the start of smb requests and responses can be enabled via:
 
 	echo 1 > /proc/fs/cifs/traceSMB
 
-Two other experimental features are under development. To test these
-requires enabling CONFIG_CIFS_EXPERIMENTAL
-
-	cifsacl support needed to retrieve approximated mode bits based on
-		the contents on the CIFS ACL.
-
-	lease support: cifs will check the oplock state before calling into
-	the vfs to see if we can grant a lease on a file.
-
-	DNOTIFY fcntl: needed for support of directory change 
-			    notification and perhaps later for file leases)
-
 Per share (per client mount) statistics are available in /proc/fs/cifs/Stats
 if the kernel was configured with cifs statistics enabled.  The statistics
 represent the number of successful (ie non-zero return code from the server) 
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index cb65499..fe77e69 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -296,12 +296,6 @@ extern int CIFSSMBUnixQuerySymLink(const int xid,
 			struct cifsTconInfo *tcon,
 			const unsigned char *searchName, char **syminfo,
 			const struct nls_table *nls_codepage);
-extern int CIFSSMBQueryReparseLinkInfo(const int xid,
-			struct cifsTconInfo *tcon,
-			const unsigned char *searchName,
-			char *symlinkinfo, const int buflen, __u16 fid,
-			const struct nls_table *nls_codepage);
-
 extern int CIFSSMBOpen(const int xid, struct cifsTconInfo *tcon,
 			const char *fileName, const int disposition,
 			const int access_flags, const int omode,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 4df1d10..9af98f6 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2474,110 +2474,6 @@ querySymLinkRetry:
 	return rc;
 }
 
-#ifdef CONFIG_CIFS_EXPERIMENTAL
-int
-CIFSSMBQueryReparseLinkInfo(const int xid, struct cifsTconInfo *tcon,
-			const unsigned char *searchName,
-			char *symlinkinfo, const int buflen, __u16 fid,
-			const struct nls_table *nls_codepage)
-{
-	int rc = 0;
-	int bytes_returned;
-	struct smb_com_transaction_ioctl_req *pSMB;
-	struct smb_com_transaction_ioctl_rsp *pSMBr;
-
-	cFYI(1, "In Windows reparse style QueryLink for path %s", searchName);
-	rc = smb_init(SMB_COM_NT_TRANSACT, 23, tcon, (void **) &pSMB,
-		      (void **) &pSMBr);
-	if (rc)
-		return rc;
-
-	pSMB->TotalParameterCount = 0 ;
-	pSMB->TotalDataCount = 0;
-	pSMB->MaxParameterCount = cpu_to_le32(2);
-	/* BB find exact data count max from sess structure BB */
-	pSMB->MaxDataCount = cpu_to_le32((tcon->ses->server->maxBuf -
-					  MAX_CIFS_HDR_SIZE) & 0xFFFFFF00);
-	pSMB->MaxSetupCount = 4;
-	pSMB->Reserved = 0;
-	pSMB->ParameterOffset = 0;
-	pSMB->DataCount = 0;
-	pSMB->DataOffset = 0;
-	pSMB->SetupCount = 4;
-	pSMB->SubCommand = cpu_to_le16(NT_TRANSACT_IOCTL);
-	pSMB->ParameterCount = pSMB->TotalParameterCount;
-	pSMB->FunctionCode = cpu_to_le32(FSCTL_GET_REPARSE_POINT);
-	pSMB->IsFsctl = 1; /* FSCTL */
-	pSMB->IsRootFlag = 0;
-	pSMB->Fid = fid; /* file handle always le */
-	pSMB->ByteCount = 0;
-
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	if (rc) {
-		cFYI(1, "Send error in QueryReparseLinkInfo = %d", rc);
-	} else {		/* decode response */
-		__u32 data_offset = le32_to_cpu(pSMBr->DataOffset);
-		__u32 data_count = le32_to_cpu(pSMBr->DataCount);
-		if ((pSMBr->ByteCount < 2) || (data_offset > 512)) {
-		/* BB also check enough total bytes returned */
-			rc = -EIO;	/* bad smb */
-			goto qreparse_out;
-		}
-		if (data_count && (data_count < 2048)) {
-			char *end_of_smb = 2 /* sizeof byte count */ +
-				pSMBr->ByteCount + (char *)&pSMBr->ByteCount;
-
-			struct reparse_data *reparse_buf =
-						(struct reparse_data *)
-						((char *)&pSMBr->hdr.Protocol
-								 + data_offset);
-			if ((char *)reparse_buf >= end_of_smb) {
-				rc = -EIO;
-				goto qreparse_out;
-			}
-			if ((reparse_buf->LinkNamesBuf +
-				reparse_buf->TargetNameOffset +
-				reparse_buf->TargetNameLen) > end_of_smb) {
-				cFYI(1, "reparse buf beyond SMB");
-				rc = -EIO;
-				goto qreparse_out;
-			}
-
-			if (pSMBr->hdr.Flags2 & SMBFLG2_UNICODE) {
-				cifs_from_ucs2(symlinkinfo, (__le16 *)
-						(reparse_buf->LinkNamesBuf +
-						reparse_buf->TargetNameOffset),
-						buflen,
-						reparse_buf->TargetNameLen,
-						nls_codepage, 0);
-			} else { /* ASCII names */
-				strncpy(symlinkinfo,
-					reparse_buf->LinkNamesBuf +
-					reparse_buf->TargetNameOffset,
-					min_t(const int, buflen,
-					   reparse_buf->TargetNameLen));
-			}
-		} else {
-			rc = -EIO;
-			cFYI(1, "Invalid return data count on "
-				 "get reparse info ioctl");
-		}
-		symlinkinfo[buflen] = 0; /* just in case so the caller
-					does not go off the end of the buffer */
-		cFYI(1, "readlink result - %s", symlinkinfo);
-	}
-
-qreparse_out:
-	cifs_buf_release(pSMB);
-
-	/* Note: On -EAGAIN error only caller can retry on handle based calls
-		since file handle passed in no longer valid */
-
-	return rc;
-}
-#endif /* CIFS_EXPERIMENTAL */
-
 #ifdef CONFIG_CIFS_POSIX
 
 /*Convert an Access Control Entry from wire format to local POSIX xattr format*/
-- 
1.7.3.2

[PATCH 4/4] cifs: replace /proc/fs/cifs/Experimental with a module parm

[Jeff Layton]

This flag currently only affects whether we allow "zero-copy" writes
with signing enabled. Typically we map pages in the pagecache directly
into the write request. If signing is enabled however and the contents
of the page change after the signature is calculated but before the
write is sent then the signature will be wrong. Servers typically
respond to this by closing down the socket.

Still, this can provide a performance benefit so the "Experimental" flag
was overloaded to allow this. That's really not a good place for this
option however since it's not clear what that flag does.

Move that flag instead to a new module parameter that better describes
its purpose. That's also better since it can be set at module insertion
time by configuring modprobe.d.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/README       |   16 ----------------
 fs/cifs/cifs_debug.c |   43 -------------------------------------------
 fs/cifs/cifsfs.c     |    6 ++++--
 fs/cifs/cifsglob.h   |    2 +-
 fs/cifs/file.c       |    4 ++--
 5 files changed, 7 insertions(+), 64 deletions(-)

diff --git a/fs/cifs/README b/fs/cifs/README
index 60a8ba5..309525a 100644
--- a/fs/cifs/README
+++ b/fs/cifs/README
@@ -680,22 +680,6 @@ LinuxExtensionsEnabled	If set to one then the client will attempt to
 			support and want to map the uid and gid fields 
 			to values supplied at mount (rather than the 
 			actual values, then set this to zero. (default 1)
-Experimental            When set to 1 used to enable certain experimental
-			features (currently enables multipage writes
-			when signing is enabled, the multipage write
-			performance enhancement was disabled when
-			signing turned on in case buffer was modified
-			just before it was sent, also this flag will
-			be used to use the new experimental directory change 
-			notification code).  When set to 2 enables
-			an additional experimental feature, "raw ntlmssp"
-			session establishment support (which allows
-			specifying "sec=ntlmssp" on mount). The Linux cifs
-			module will use ntlmv2 authentication encapsulated
-			in "raw ntlmssp" (not using SPNEGO) when
-			"sec=ntlmssp" is specified on mount.
-			This support also requires building cifs with
-			the CONFIG_CIFS_EXPERIMENTAL configuration flag.
 
 These experimental features and tracing can be enabled by changing flags in 
 /proc/fs/cifs (after the cifs module has been installed or built into the 
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 103ab8b..37f7534 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -425,7 +425,6 @@ static const struct file_operations cifs_lookup_cache_proc_fops;
 static const struct file_operations traceSMB_proc_fops;
 static const struct file_operations cifs_multiuser_mount_proc_fops;
 static const struct file_operations cifs_security_flags_proc_fops;
-static const struct file_operations cifs_experimental_proc_fops;
 static const struct file_operations cifs_linux_ext_proc_fops;
 
 void
@@ -443,8 +442,6 @@ cifs_proc_init(void)
 	proc_create("cifsFYI", 0, proc_fs_cifs, &cifsFYI_proc_fops);
 	proc_create("traceSMB", 0, proc_fs_cifs, &traceSMB_proc_fops);
 	proc_create("OplockEnabled", 0, proc_fs_cifs, &cifs_oplock_proc_fops);
-	proc_create("Experimental", 0, proc_fs_cifs,
-		    &cifs_experimental_proc_fops);
 	proc_create("LinuxExtensionsEnabled", 0, proc_fs_cifs,
 		    &cifs_linux_ext_proc_fops);
 	proc_create("MultiuserMount", 0, proc_fs_cifs,
@@ -471,7 +468,6 @@ cifs_proc_clean(void)
 	remove_proc_entry("OplockEnabled", proc_fs_cifs);
 	remove_proc_entry("SecurityFlags", proc_fs_cifs);
 	remove_proc_entry("LinuxExtensionsEnabled", proc_fs_cifs);
-	remove_proc_entry("Experimental", proc_fs_cifs);
 	remove_proc_entry("LookupCacheEnabled", proc_fs_cifs);
 	remove_proc_entry("fs/cifs", NULL);
 }
@@ -552,45 +548,6 @@ static const struct file_operations cifs_oplock_proc_fops = {
 	.write		= cifs_oplock_proc_write,
 };
 
-static int cifs_experimental_proc_show(struct seq_file *m, void *v)
-{
-	seq_printf(m, "%d\n", experimEnabled);
-	return 0;
-}
-
-static int cifs_experimental_proc_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, cifs_experimental_proc_show, NULL);
-}
-
-static ssize_t cifs_experimental_proc_write(struct file *file,
-		const char __user *buffer, size_t count, loff_t *ppos)
-{
-	char c;
-	int rc;
-
-	rc = get_user(c, buffer);
-	if (rc)
-		return rc;
-	if (c == '0' || c == 'n' || c == 'N')
-		experimEnabled = 0;
-	else if (c == '1' || c == 'y' || c == 'Y')
-		experimEnabled = 1;
-	else if (c == '2')
-		experimEnabled = 2;
-
-	return count;
-}
-
-static const struct file_operations cifs_experimental_proc_fops = {
-	.owner		= THIS_MODULE,
-	.open		= cifs_experimental_proc_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-	.write		= cifs_experimental_proc_write,
-};
-
 static int cifs_linux_ext_proc_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "%d\n", linuxExtEnabled);
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 48582b7..1f5fec1 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -53,7 +53,6 @@ int cifsFYI = 0;
 int cifsERROR = 1;
 int traceSMB = 0;
 unsigned int oplockEnabled = 1;
-unsigned int experimEnabled = 0;
 unsigned int linuxExtEnabled = 1;
 unsigned int lookupCacheEnabled = 1;
 unsigned int multiuser_mount = 0;
@@ -77,7 +76,10 @@ unsigned int cifs_max_pending = CIFS_MAX_REQ;
 module_param(cifs_max_pending, int, 0);
 MODULE_PARM_DESC(cifs_max_pending, "Simultaneous requests to server. "
 				   "Default: 50 Range: 2 to 256");
-
+bool sign_zero_copy = false;
+module_param(sign_zero_copy, bool, 0644);
+MODULE_PARM_DESC(sign_zero_copy, "Don't copy pages on write with signing "
+				 "enabled. Default: N");
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 37fb246..cf0dfda 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -777,7 +777,6 @@ GLOBAL_EXTERN unsigned int multiuser_mount; /* if enabled allows new sessions
 				have the uid/password or Kerberos credential
 				or equivalent for current user */
 GLOBAL_EXTERN unsigned int oplockEnabled;
-GLOBAL_EXTERN unsigned int experimEnabled;
 GLOBAL_EXTERN unsigned int lookupCacheEnabled;
 GLOBAL_EXTERN unsigned int global_secflags;	/* if on, session setup sent
 				with more secure ntlmssp2 challenge/resp */
@@ -787,6 +786,7 @@ GLOBAL_EXTERN unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 GLOBAL_EXTERN unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
 GLOBAL_EXTERN unsigned int cifs_min_small;  /* min size of small buf pool */
 GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/
+GLOBAL_EXTERN bool sign_zero_copy; /* don't copy written pages with signing */
 
 void cifs_oplock_break(struct work_struct *work);
 void cifs_oplock_break_get(struct cifsFileInfo *cfile);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5a28660..fe16f6d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1054,7 +1054,7 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 				if (rc != 0)
 					break;
 			}
-			if (experimEnabled || (pTcon->ses->server &&
+			if (sign_zero_copy || (pTcon->ses->server &&
 				((pTcon->ses->server->secMode &
 				(SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED))
 				== 0))) {
@@ -1319,7 +1319,7 @@ static int cifs_writepages(struct address_space *mapping,
 	}
 
 	tcon = tlink_tcon(open_file->tlink);
-	if (!experimEnabled && tcon->ses->server->secMode &
+	if (!sign_zero_copy && tcon->ses->server->secMode &
 			(SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
 		cifsFileInfo_put(open_file);
 		kfree(iov);
-- 
1.7.3.2

Re: [PATCH 0/4] cifs: CONFIG_CIFS_EXPERIMENTAL removal (try #2)

[Suresh Jayaraman]

On 12/08/2010 08:33 PM, Jeff Layton wrote:
> This is the second version of this patchset. The changes since the last
> set are:
> 
> 1) The patch to remove "/proc/fs/cifs/Experimental" did not remove the
> deregistration of that file, which caused a warning on rmmod.
> 
> 2) The patch to remove "/proc/fs/cifs/Experimental" now adds a new
> module parameter so that people relying on it to allow zero-copy
> writes with signing have a way to continue using that.
> 
> A modified description of the set follows...
> 
> The CONFIG_CIFS_EXPERIMENTAL KConfig option is the sort of thing that
> gives distro packagers nightmares. The things that live under it are
> impossible to predict for someone who isn't following development
> upstream.

In general, I like the overall idea of removing heavily overloaded
CIFS_EXPERIMENTAL config option. It's true that it was at times hard to
narrow down suspect once this option is enabled.

However, the dependency of a few options on EXPERIMENTAL (fscache and
acl) is not removed. CIFS_FSCACHE can be marked as not dependent on
EXPERIMENTAL. Not sure about CIFS_ACL though.

Can you make this patchset or a subsequent patch accomodates this change
too?


-- 
Suresh Jayaraman

Re: [PATCH 1/4] cifs: remove export_ops code

[Suresh Jayaraman]

On 12/08/2010 08:33 PM, Jeff Layton wrote:
> Stubs for this code were added under the CONFIG_CIFS_EXPERIMENTAL tag in
> 2007, but were never completed. The upshot -- anyone who turns on
> CONFIG_CIFS_EXPERIMENTAL gets a filesystem that pretends to allow exporting
> via nfsd. Trying to actually use it will just get you an error once
> nfsd tries to walk into the directory.
> 
> Remove this code since it's unfinished. We can reintroduce it once someone
> spends time on it and makes it usable.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/Makefile |    2 +-
>  fs/cifs/cifsfs.c |    7 -----
>  fs/cifs/cifsfs.h |    4 ---
>  fs/cifs/export.c |   67 ------------------------------------------------------
>  4 files changed, 1 insertions(+), 79 deletions(-)
>  delete mode 100644 fs/cifs/export.c

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Perhaps it would be a good idea to briefly document expected behavior,
what is working, problems etc (in Samba wiki) so that someone can pick
up this later?

Re: [PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Suresh Jayaraman]

On 12/08/2010 08:33 PM, Jeff Layton wrote:
> I see no real need to leave these sorts of options under an
> EXPERIMENTAL ifdef. Since you need a mount option to turn this code
> on, that only blows out the testing matrix.
> 
> local_leases has been under the EXPERIMENTAL tag for some time, but
> it's only the mount option that's under this label. Move it out
> from under this tag.
> 
> The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
> option to turn it on, and in the future any distro will reasonably
> want this enabled. Go ahead and move it out from under the
> EXPERIMENTAL tag.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifssmb.c |    5 +--
>  fs/cifs/connect.c |    4 --
>  fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
>  3 files changed, 48 insertions(+), 64 deletions(-)

Moving local_leases out sounds fine to me. While I agree that NTLMSSP is
something everyone wants moving ahead, I'm not sure whether there are
currently any known/open issues with specific Servers.
Shirish?

Assuming there are none,

Acked-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 3/4] cifs: remove CIFSSMBQueryReparseLinkInfo and CONFIG_CIFS_EXPERIMENTAL

[Suresh Jayaraman]

On 12/08/2010 08:33 PM, Jeff Layton wrote:
> Nothing calls this function. Remove it.
> 
> With this, the last user of CONFIG_CIFS_EXPERIMENTAL has been
> removed. Also, update the README.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/Kconfig     |   13 ------
>  fs/cifs/README      |   12 ------
>  fs/cifs/cifsproto.h |    6 ---
>  fs/cifs/cifssmb.c   |  104 ---------------------------------------------------
>  4 files changed, 0 insertions(+), 135 deletions(-)
> 

Looks good to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 4/4] cifs: replace /proc/fs/cifs/Experimental with a module parm

[Suresh Jayaraman]

On 12/08/2010 08:33 PM, Jeff Layton wrote:
> This flag currently only affects whether we allow "zero-copy" writes
> with signing enabled. Typically we map pages in the pagecache directly
> into the write request. If signing is enabled however and the contents
> of the page change after the signature is calculated but before the
> write is sent then the signature will be wrong. Servers typically
> respond to this by closing down the socket.
> 
> Still, this can provide a performance benefit so the "Experimental" flag
> was overloaded to allow this. That's really not a good place for this
> option however since it's not clear what that flag does.
> 
> Move that flag instead to a new module parameter that better describes
> its purpose. That's also better since it can be set at module insertion
> time by configuring modprobe.d.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/README       |   16 ----------------
>  fs/cifs/cifs_debug.c |   43 -------------------------------------------
>  fs/cifs/cifsfs.c     |    6 ++++--
>  fs/cifs/cifsglob.h   |    2 +-
>  fs/cifs/file.c       |    4 ++--
>  5 files changed, 7 insertions(+), 64 deletions(-)
> 

Making it a module param sounds appropriate to me.

Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH 0/4] cifs: CONFIG_CIFS_EXPERIMENTAL removal (try #2)

[Jeff Layton]

On Fri, 17 Dec 2010 20:58:22 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/08/2010 08:33 PM, Jeff Layton wrote:
> > This is the second version of this patchset. The changes since the last
> > set are:
> > 
> > 1) The patch to remove "/proc/fs/cifs/Experimental" did not remove the
> > deregistration of that file, which caused a warning on rmmod.
> > 
> > 2) The patch to remove "/proc/fs/cifs/Experimental" now adds a new
> > module parameter so that people relying on it to allow zero-copy
> > writes with signing have a way to continue using that.
> > 
> > A modified description of the set follows...
> > 
> > The CONFIG_CIFS_EXPERIMENTAL KConfig option is the sort of thing that
> > gives distro packagers nightmares. The things that live under it are
> > impossible to predict for someone who isn't following development
> > upstream.
> 
> In general, I like the overall idea of removing heavily overloaded
> CIFS_EXPERIMENTAL config option. It's true that it was at times hard to
> narrow down suspect once this option is enabled.
> 
> However, the dependency of a few options on EXPERIMENTAL (fscache and
> acl) is not removed. CIFS_FSCACHE can be marked as not dependent on
> EXPERIMENTAL. Not sure about CIFS_ACL though.
> 
> Can you make this patchset or a subsequent patch accomodates this change
> too?
> 
> 

There's a difference between CONFIG_CIFS_EXPERIMENTAL and
CONFIG_EXPERIMENTAL. Those options depend on the latter. I'm removing
the former. I think it's still appropriate to mark these new features
as EXPERIMENTAL and to leave them dependent on CONFIG_EXPERIMENTAL.

Note, that I don't necessarily think we must leave these features
tagged CONFIG_EXPERIMENTAL just that if we want to remove those labels,
we should do that in the context of a separate discussion...

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/4] cifs: remove export_ops code

[Jeff Layton]

On Fri, 17 Dec 2010 20:58:42 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/08/2010 08:33 PM, Jeff Layton wrote:
> > Stubs for this code were added under the CONFIG_CIFS_EXPERIMENTAL tag in
> > 2007, but were never completed. The upshot -- anyone who turns on
> > CONFIG_CIFS_EXPERIMENTAL gets a filesystem that pretends to allow exporting
> > via nfsd. Trying to actually use it will just get you an error once
> > nfsd tries to walk into the directory.
> > 
> > Remove this code since it's unfinished. We can reintroduce it once someone
> > spends time on it and makes it usable.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/Makefile |    2 +-
> >  fs/cifs/cifsfs.c |    7 -----
> >  fs/cifs/cifsfs.h |    4 ---
> >  fs/cifs/export.c |   67 ------------------------------------------------------
> >  4 files changed, 1 insertions(+), 79 deletions(-)
> >  delete mode 100644 fs/cifs/export.c
> 
> Looks good to me.
> 
> Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> 
> Perhaps it would be a good idea to briefly document expected behavior,
> what is working, problems etc (in Samba wiki) so that someone can pick
> up this later?
> 

Sounds reasonable.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Jeff Layton]

On Fri, 17 Dec 2010 20:58:56 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/08/2010 08:33 PM, Jeff Layton wrote:
> > I see no real need to leave these sorts of options under an
> > EXPERIMENTAL ifdef. Since you need a mount option to turn this code
> > on, that only blows out the testing matrix.
> > 
> > local_leases has been under the EXPERIMENTAL tag for some time, but
> > it's only the mount option that's under this label. Move it out
> > from under this tag.
> > 
> > The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
> > option to turn it on, and in the future any distro will reasonably
> > want this enabled. Go ahead and move it out from under the
> > EXPERIMENTAL tag.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/cifssmb.c |    5 +--
> >  fs/cifs/connect.c |    4 --
> >  fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
> >  3 files changed, 48 insertions(+), 64 deletions(-)
> 
> Moving local_leases out sounds fine to me. While I agree that NTLMSSP is
> something everyone wants moving ahead, I'm not sure whether there are
> currently any known/open issues with specific Servers.
> Shirish?
> 
> Assuming there are none,
> 
> Acked-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

I think the NTLMSSP code is still a work in progress. There are still
some bugs, but Shirish has really fixed and cleaned the code up
recently. At this point, it works well enough that there's not much
benefit to keeping it under an EXPERIMENTAL tag. That just makes it
hard to integrate the code. It's messy and you have to deal with two
different compile-time scenarios. I'd feel differently if were code
that changed the "normal" functioning of CIFS, but since you have to
use a mount option to turn this, I think we should just go for it.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Shirish Pargaonkar]

On Fri, Dec 17, 2010 at 9:28 AM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> On 12/08/2010 08:33 PM, Jeff Layton wrote:
>> I see no real need to leave these sorts of options under an
>> EXPERIMENTAL ifdef. Since you need a mount option to turn this code
>> on, that only blows out the testing matrix.
>>
>> local_leases has been under the EXPERIMENTAL tag for some time, but
>> it's only the mount option that's under this label. Move it out
>> from under this tag.
>>
>> The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
>> option to turn it on, and in the future any distro will reasonably
>> want this enabled. Go ahead and move it out from under the
>> EXPERIMENTAL tag.
>>
>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/cifssmb.c |    5 +--
>>  fs/cifs/connect.c |    4 --
>>  fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
>>  3 files changed, 48 insertions(+), 64 deletions(-)
>
> Moving local_leases out sounds fine to me. While I agree that NTLMSSP is
> something everyone wants moving ahead, I'm not sure whether there are
> currently any known/open issues with specific Servers.
> Shirish?
>
> Assuming there are none,

Yes. There was one issue in NTLMSSP and it has been fixed and
the patch hasbeen posted, discussed, and ack'ed by Jeff Layton.
I am not aware of any other issues at this time that break
NTLMSSP authentication.

Only change we would want to see is reconciliation of flags sent
by client in type 1 (negotiation) exchange packet and flags sent
by server in type 2 (challenge) exchange packet and resultant
flags and data according to those flags sent by client in
type 3 (authentication) exchange packet.

>
> Acked-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>

Re: [PATCH 0/4] cifs: CONFIG_CIFS_EXPERIMENTAL removal (try #2)

[Suresh Jayaraman]

On 12/17/2010 09:07 PM, Jeff Layton wrote:
> On Fri, 17 Dec 2010 20:58:22 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 12/08/2010 08:33 PM, Jeff Layton wrote:
>>> This is the second version of this patchset. The changes since the last
>>> set are:
>>>
>>> 1) The patch to remove "/proc/fs/cifs/Experimental" did not remove the
>>> deregistration of that file, which caused a warning on rmmod.
>>>
>>> 2) The patch to remove "/proc/fs/cifs/Experimental" now adds a new
>>> module parameter so that people relying on it to allow zero-copy
>>> writes with signing have a way to continue using that.
>>>
>>> A modified description of the set follows...
>>>
>>> The CONFIG_CIFS_EXPERIMENTAL KConfig option is the sort of thing that
>>> gives distro packagers nightmares. The things that live under it are
>>> impossible to predict for someone who isn't following development
>>> upstream.
>>
>> In general, I like the overall idea of removing heavily overloaded
>> CIFS_EXPERIMENTAL config option. It's true that it was at times hard to
>> narrow down suspect once this option is enabled.
>>
>> However, the dependency of a few options on EXPERIMENTAL (fscache and
>> acl) is not removed. CIFS_FSCACHE can be marked as not dependent on
>> EXPERIMENTAL. Not sure about CIFS_ACL though.
>>
>> Can you make this patchset or a subsequent patch accomodates this change
>> too?
>>
> 
> There's a difference between CONFIG_CIFS_EXPERIMENTAL and
> CONFIG_EXPERIMENTAL. Those options depend on the latter. I'm removing
> the former. I think it's still appropriate to mark these new features
> as EXPERIMENTAL and to leave them dependent on CONFIG_EXPERIMENTAL.

Doh, I missed it. Indeed they are different. Sorry about the noise.

> Note, that I don't necessarily think we must leave these features
> tagged CONFIG_EXPERIMENTAL just that if we want to remove those labels,
> we should do that in the context of a separate discussion...
> 

Agreed.

-- 
Suresh Jayaraman

Re: [PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Steve French]

I would like this patch in soon - as was noted it simplifies
build/testing to make these options non-experimental and we need to
have at least an informal schedule for moving code out of experimental
when no bugs found (over a few release cycles)

Shirish,
Can you point me to any NTLMSSP fixes you think are necessary?

On Fri, Dec 17, 2010 at 9:43 AM, Shirish Pargaonkar
<shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Dec 17, 2010 at 9:28 AM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>> On 12/08/2010 08:33 PM, Jeff Layton wrote:
>>> I see no real need to leave these sorts of options under an
>>> EXPERIMENTAL ifdef. Since you need a mount option to turn this code
>>> on, that only blows out the testing matrix.
>>>
>>> local_leases has been under the EXPERIMENTAL tag for some time, but
>>> it's only the mount option that's under this label. Move it out
>>> from under this tag.
>>>
>>> The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
>>> option to turn it on, and in the future any distro will reasonably
>>> want this enabled. Go ahead and move it out from under the
>>> EXPERIMENTAL tag.
>>>
>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  fs/cifs/cifssmb.c |    5 +--
>>>  fs/cifs/connect.c |    4 --
>>>  fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
>>>  3 files changed, 48 insertions(+), 64 deletions(-)
>>
>> Moving local_leases out sounds fine to me. While I agree that NTLMSSP is
>> something everyone wants moving ahead, I'm not sure whether there are
>> currently any known/open issues with specific Servers.
>> Shirish?
>>
>> Assuming there are none,
>
> Yes. There was one issue in NTLMSSP and it has been fixed and
> the patch hasbeen posted, discussed, and ack'ed by Jeff Layton.
> I am not aware of any other issues at this time that break
> NTLMSSP authentication.
>
> Only change we would want to see is reconciliation of flags sent
> by client in type 1 (negotiation) exchange packet and flags sent
> by server in type 2 (challenge) exchange packet and resultant
> flags and data according to those flags sent by client in
> type 3 (authentication) exchange packet.
>
>>
>> Acked-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>>
>



-- 
Thanks,

Steve

Re: [PATCH 2/4] cifs: move "ntlmssp" and "local_leases" options out of experimental code

[Shirish Pargaonkar]

On Fri, Dec 17, 2010 at 10:33 AM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> I would like this patch in soon - as was noted it simplifies
> build/testing to make these options non-experimental and we need to
> have at least an informal schedule for moving code out of experimental
> when no bugs found (over a few release cycles)
>
> Shirish,
> Can you point me to any NTLMSSP fixes you think are necessary?

Steve, this patch is ready to get merged. I think it would be helpful
to have it in
as soon as possible, has been Ack'ed by Jeff Layton and tested by
Robbert Kouprie
(and I did verify too that it works).

[PATCH] cifs: Support NTLM2 session security during NTLMSSP
authentication [try #5]

>
> On Fri, Dec 17, 2010 at 9:43 AM, Shirish Pargaonkar
> <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, Dec 17, 2010 at 9:28 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>> On 12/08/2010 08:33 PM, Jeff Layton wrote:
>>>> I see no real need to leave these sorts of options under an
>>>> EXPERIMENTAL ifdef. Since you need a mount option to turn this code
>>>> on, that only blows out the testing matrix.
>>>>
>>>> local_leases has been under the EXPERIMENTAL tag for some time, but
>>>> it's only the mount option that's under this label. Move it out
>>>> from under this tag.
>>>>
>>>> The NTLMSSP code is also under EXPERIMENTAL, but it needs a mount
>>>> option to turn it on, and in the future any distro will reasonably
>>>> want this enabled. Go ahead and move it out from under the
>>>> EXPERIMENTAL tag.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  fs/cifs/cifssmb.c |    5 +--
>>>>  fs/cifs/connect.c |    4 --
>>>>  fs/cifs/sess.c    |  103 ++++++++++++++++++++++++-----------------------------
>>>>  3 files changed, 48 insertions(+), 64 deletions(-)
>>>
>>> Moving local_leases out sounds fine to me. While I agree that NTLMSSP is
>>> something everyone wants moving ahead, I'm not sure whether there are
>>> currently any known/open issues with specific Servers.
>>> Shirish?
>>>
>>> Assuming there are none,
>>
>> Yes. There was one issue in NTLMSSP and it has been fixed and
>> the patch hasbeen posted, discussed, and ack'ed by Jeff Layton.
>> I am not aware of any other issues at this time that break
>> NTLMSSP authentication.
>>
>> Only change we would want to see is reconciliation of flags sent
>> by client in type 1 (negotiation) exchange packet and flags sent
>> by server in type 2 (challenge) exchange packet and resultant
>> flags and data according to those flags sent by client in
>> type 3 (authentication) exchange packet.
>>
>>>
>>> Acked-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>>>
>>
>
>
>
> --
> Thanks,
>
> Steve
>