Re: [RFC PATCH 24/31] cifs: Convert SMB2 Negotiate Protocol request

[David Howells <dhowells@redhat.com>]

Enzo Matsumiya <ematsumiya@suse.de> wrote:

> On 08/06, David Howells wrote:
> > ...
> > -static unsigned int
> >-build_netname_ctxt(struct smb2_netname_neg_context *pneg_ctxt, char *hostname)
> >+static size_t smb2_size_netname_ctxt(struct TCP_Server_Info *server)
> > {
> >+	size_t data_len;
> >+
> >+#if 0
> > 	struct nls_table *cp = load_nls_default();
> >+	const char *hostname;
> >
> >-	pneg_ctxt->ContextType = SMB2_NETNAME_NEGOTIATE_CONTEXT_ID;
> >+	/* Only include up to first 100 bytes of server name in the NetName
> >+	 * field.
> >+	 */
> >+	cifs_server_lock(pserver);
> >+	hostname = pserver->hostname;
> >+	if (hostname && hostname[0])
> >+		data_len = cifs_size_strtoUTF16(hostname, 100, cp);
> >+	cifs_server_unlock(pserver);
> >+#else
> >+	/* Now, we can't just measure the length of hostname as, unless we hold
> >+	 * the lock, it may change under us, so allow maximum space for it.
> >+	 */
> >+	data_len = 400;
> >+#endif
> >+	return ALIGN8(sizeof(struct smb2_neg_context) + data_len);
> >+}
> 
> Why was this commented out?  Your comment implies that you can't hold
> the lock anymore there, but I couldn't find out why (with your patches
> applied).

The problem is that the hostname may change - and there's a spinlock to
protect it.  However, now that I'm working out the message size before the
allocation, I need to find the size of the host name, do the alloc and then
copy the hostname in - but I can't hold the spinlock across the alloc, so the
hostname may change whilst the lock is dropped.

The obvious solution is to just allocate the maximum size for it.  It's not
that big and this command isn't used all that often.

Remember that this is a work in progress, so you may find bits like this where
I may need to reconsider what I've chosen.

> >-static void
> >-assemble_neg_contexts(struct smb2_negotiate_req *req,
> >-		      struct TCP_Server_Info *server, unsigned int *total_len)
> >+static size_t smb2_size_neg_contexts(struct TCP_Server_Info *server,
> >+				     size_t offset)
> > {
> >-	unsigned int ctxt_len, neg_context_count;
> > 	struct TCP_Server_Info *pserver;
> >-	char *pneg_ctxt;
> >-	char *hostname;
> >-
> >-	if (*total_len > 200) {
> >-		/* In case length corrupted don't want to overrun smb buffer */
> >-		cifs_server_dbg(VFS, "Bad frame length assembling neg contexts\n");
> >-		return;
> >-	}
> >
> > 	/*
> > 	 * round up total_len of fixed part of SMB3 negotiate request to 8
> > 	 * byte boundary before adding negotiate contexts
> > 	 */
> >-	*total_len = ALIGN8(*total_len);
> >+	offset = ALIGN8(offset);
> >+	offset += ALIGN8(sizeof(struct smb2_preauth_neg_context));
> >+	offset += ALIGN8(sizeof(struct smb2_encryption_neg_context));
> >
> >-	pneg_ctxt = (*total_len) + (char *)req;
> >-	req->NegotiateContextOffset = cpu_to_le32(*total_len);
> >+	/*
> >+	 * secondary channels don't have the hostname field populated
> >+	 * use the hostname field in the primary channel instead
> >+	 */
> >+	pserver = SERVER_IS_CHAN(server) ? server->primary_server : server;
> >+	offset += smb2_size_netname_ctxt(pserver);
> 
> If you're keeping data_len=400 above, you could just drop
> smb2_size_netname_ctxt() altogether and use
> "ALIGN8(sizeof(struct smb2_neg_context) + 400)" directly here.

Yeah.  Probably would make sense to do that with a comment saying why 400.

David