Re: [PATCH] Remove BKL usage from ncpfs

[Petr Vandrovec <petr@vandrovec.name>]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

On Mon, Sep 27, 2010 at 7:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 27 September 2010, Petr Vandrovec wrote:
>> commit 92498b5267aa58e85e20c7b2cbd84d1ed86df47d
>> Author: Petr Vandrovec <petr@vandrovec.name>
>> Date:   Sun Sep 26 16:19:12 2010 -0700
>>
>>     Remove BKL from ncpfs
>
> Hi Petr,
>
> Thanks for taking care of this.
>
> Would you like me to take this patch into my bkl/vfs tree? I think
> that would make it easier for me because I'm adding another instance
> of the BKL there, in the ncp_fill_super function. As far as I can tell
> that change (see below) becomes pointless with your patch.

Thanks.  I would appreciate it.

> I could either just revert my change or replace it with your patch,
> whichever you prefer.

Yes, I believe it is not necessary.  fill_super(sb) should not run
concurrently with anything else because MS_BORN and MS_ACTIVE are not
set yet so nobody else should use this sb from VFS.  One thing which
seems to be missing is doing lock_sock() around code which sets
sk->sk_{error_report,data_ready,write_space} - there does not seem to
be anything else to protect ipv4/ipv6/ipx from seeing partially
updated pointers on systems where these writes are not atomic - that's
ncpfs2.patch.

Also I found some whitespace problems, and one missing const, so if
you could merge ncpfs3.patch & ncpfs4.patch with original BKL removal,
it would be cool.  Or I can resend all 4 patches as one bigger diff if
you prefer.

Thanks,
Petr

[-- Attachment #2: ncpfs2.patch --]
[-- Type: application/octet-stream, Size: 1947 bytes --]

Lock socket in ncpfs while setting its callbacks

Otherwise partially updated pointers could be seen if
pointer update is not atomic.

Signed-off-by: Petr Vandrovec <petr@vandrovec.name>

diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
index 5f4e58d..985fabb 100644
--- a/fs/ncpfs/inode.c
+++ b/fs/ncpfs/inode.c
@@ -303,10 +303,12 @@ ncp_evict_inode(struct inode *inode)
 
 static void ncp_stop_tasks(struct ncp_server *server) {
 	struct sock* sk = server->ncp_sock->sk;
-		
+
+	lock_sock(sk);
 	sk->sk_error_report = server->error_report;
 	sk->sk_data_ready   = server->data_ready;
 	sk->sk_write_space  = server->write_space;
+	release_sock(sk);
 	del_timer_sync(&server->timeout_tm);
 	flush_scheduled_work();
 }
@@ -605,10 +607,6 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 	mutex_init(&server->rcv.creq_mutex);
 	server->tx.creq		= NULL;
 	server->rcv.creq	= NULL;
-	server->data_ready	= sock->sk->sk_data_ready;
-	server->write_space	= sock->sk->sk_write_space;
-	server->error_report	= sock->sk->sk_error_report;
-	sock->sk->sk_user_data	= server;
 
 	init_timer(&server->timeout_tm);
 #undef NCP_PACKET_SIZE
@@ -625,6 +623,11 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 	if (server->rxbuf == NULL)
 		goto out_txbuf;
 
+	lock_sock(sock->sk);
+	server->data_ready	= sock->sk->sk_data_ready;
+	server->write_space	= sock->sk->sk_write_space;
+	server->error_report	= sock->sk->sk_error_report;
+	sock->sk->sk_user_data	= server;
 	sock->sk->sk_data_ready	  = ncp_tcp_data_ready;
 	sock->sk->sk_error_report = ncp_tcp_error_report;
 	if (sock->type == SOCK_STREAM) {
@@ -640,6 +643,7 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 		server->timeout_tm.data = (unsigned long)server;
 		server->timeout_tm.function = ncpdgram_timeout_call;
 	}
+	release_sock(sock->sk);
 
 	ncp_lock_server(server);
 	error = ncp_connect(server);

[-- Attachment #3: ncpfs3.patch --]
[-- Type: application/octet-stream, Size: 3527 bytes --]

Remove some whitespace from ncpfs

Few whitespace errors sneaked in in last diff.  Remove them.

Signed-off-by: Petr Vandrovec <petr@vandrovec.name>

diff --git a/fs/ncpfs/ioctl.c b/fs/ncpfs/ioctl.c
index da9d0e0..c2a1f9a 100644
--- a/fs/ncpfs/ioctl.c
+++ b/fs/ncpfs/ioctl.c
@@ -304,7 +304,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 		server->current_size = request.size;
 		memcpy(server->packet, bouncebuffer, request.size);
 
-		result = ncp_request2(server, request.function, 
+		result = ncp_request2(server, request.function,
 			bouncebuffer, NCP_PACKET_SIZE_INTERNAL);
 		if (result < 0)
 			result = -EIO;
@@ -378,7 +378,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 
 				if (dentry) {
 					struct inode* s_inode = dentry->d_inode;
-				
+
 					if (s_inode) {
 						sr.volNumber = NCP_FINFO(s_inode)->volNumber;
 						sr.dirEntNum = NCP_FINFO(s_inode)->dirEntNum;
@@ -433,7 +433,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 					dentry = inode->i_sb->s_root;
 					if (dentry) {
 						struct inode* s_inode = dentry->d_inode;
-				
+
 						if (s_inode) {
 							NCP_FINFO(s_inode)->volNumber = vnum;
 							NCP_FINFO(s_inode)->dirEntNum = de;
@@ -455,7 +455,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 			return result;
 		}
 
-#ifdef CONFIG_NCPFS_PACKET_SIGNING	
+#ifdef CONFIG_NCPFS_PACKET_SIGNING
 	case NCP_IOC_SIGN_INIT:
 		{
 			struct ncp_sign_init sign;
@@ -477,14 +477,14 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 			}
 			mutex_unlock(&server->rcv.creq_mutex);
 			ncp_unlock_server(server);
-			return 0;		
+			return 0;
 		}
-		
+
         case NCP_IOC_SIGN_WANTED:
 		{
 			int state;
 
-			ncp_lock_server(server);	
+			ncp_lock_server(server);
 			state = server->sign_wanted;
 			ncp_unlock_server(server);
 			if (put_user(state, (int __user *)argp))
@@ -550,7 +550,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 			if (rqdata.cmd == NCP_LOCK_CLEAR)
 			{
 				result = ncp_ClearPhysicalRecord(NCP_SERVER(inode),
-							NCP_FINFO(inode)->file_handle, 
+							NCP_FINFO(inode)->file_handle,
 							rqdata.offset,
 							rqdata.length);
 				if (result > 0) result = 0;	/* no such lock */
@@ -573,7 +573,7 @@ static long __ncp_ioctl(struct inode *inode, unsigned int cmd, unsigned long arg
 							rqdata.timeout);
 				if (result > 0) result = -EAGAIN;
 			}
-outrel:			
+outrel:
 			ncp_inode_close(inode);
 			return result;
 		}
@@ -777,7 +777,7 @@ outrel:
 #ifdef CONFIG_NCPFS_NLS
 	case NCP_IOC_SETCHARSETS:
 		return ncp_set_charsets(server, argp);
-		
+
 	case NCP_IOC_GETCHARSETS:
 		return ncp_get_charsets(server, argp);
 
@@ -796,7 +796,7 @@ outrel:
 			atomic_set(&server->dentry_ttl, user);
 			return 0;
 		}
-		
+
 	case NCP_IOC_GETDENTRYTTL:
 		{
 			u_int32_t user = (atomic_read(&server->dentry_ttl) * 1000) / HZ;
diff --git a/fs/ncpfs/ncplib_kernel.c b/fs/ncpfs/ncplib_kernel.c
index 7bcc99d..a95615a 100644
--- a/fs/ncpfs/ncplib_kernel.c
+++ b/fs/ncpfs/ncplib_kernel.c
@@ -578,7 +578,7 @@ ncp_mount_subdir(struct ncp_server *server,
 	int dstNS;
 	int result;
 
-	ncp_update_known_namespace(server, volNumber, &dstNS);	
+	ncp_update_known_namespace(server, volNumber, &dstNS);
 	if ((result = ncp_ObtainSpecificDirBase(server, srcNS, dstNS, volNumber, 
 				      dirEntNum, NULL, newDirEnt, newDosEnt)) != 0)
 	{
 

[-- Attachment #4: ncpfs4.patch --]
[-- Type: application/octet-stream, Size: 614 bytes --]

Missing const qualifier in ncpfs

One more place which deserves char* to be const char *.

Signed-off-by: Petr Vandrovec <petr@vandrovec.name>

diff --git a/fs/ncpfs/ncplib_kernel.c b/fs/ncpfs/ncplib_kernel.c
index 7bcc99d..a95615a 100644
--- a/fs/ncpfs/ncplib_kernel.c
+++ b/fs/ncpfs/ncplib_kernel.c
@@ -709,7 +709,7 @@ int ncp_modify_nfs_info(struct ncp_server *server, __u8 volnum, __le32 dirent,
 static int
 ncp_DeleteNSEntry(struct ncp_server *server,
 		  __u8 have_dir_base, __u8 volnum, __le32 dirent,
-		  char* name, __u8 ns, __le16 attr)
+		  const char* name, __u8 ns, __le16 attr)
 {
 	int result;