[v2,3/6] CIFS: implement get_dfs_refer for SMB2+
diff mbox

Message ID 20170223144334.22320-4-aaptel@suse.com
State New
Headers show

Commit Message

Aurelien Aptel Feb. 23, 2017, 2:43 p.m. UTC
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/smb2ops.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/smb2pdu.h |  8 +++++++
 2 files changed, 80 insertions(+)

Comments

Pavel Shilovsky Feb. 24, 2017, 1:06 a.m. UTC | #1
2017-02-23 6:43 GMT-08:00 Aurelien Aptel <aaptel@suse.com>:
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/smb2ops.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/smb2pdu.h |  8 +++++++
>  2 files changed, 80 insertions(+)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index a44b4db..2563fe8 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1097,6 +1097,78 @@ smb2_new_lease_key(struct cifs_fid *fid)
>         generate_random_uuid(fid->lease_key);
>  }
>
> +static int
> +smb2_get_dfs_refer(const unsigned int xid, struct cifs_ses *ses,
> +                  const char *search_name,
> +                  struct dfs_info3_param **target_nodes,
> +                  unsigned int *num_of_nodes,
> +                  const struct nls_table *nls_codepage, int remap)
> +{
> +       int rc = -EINVAL;
> +       __le16 *utf16_path = NULL;
> +       int utf16_path_len = 0;
> +       struct cifs_tcon *tcon;
> +       struct fsctl_get_dfs_referral_req *dfs_req = NULL;
> +       struct get_dfs_referral_rsp *dfs_rsp = NULL;
> +       u32 dfs_req_size = 0, dfs_rsp_size = 0;
> +
> +       cifs_dbg(FYI, "smb2_get_dfs_refer path <%s>\n", search_name);
> +
> +       /*
> +        * Use any tcon from the current session. Here, the first one.
> +        */
> +       tcon = list_first_entry(&ses->tcon_list, struct cifs_tcon, tcon_list);
> +       if (!tcon)
> +               goto out;

Accessing the tcon_list should be protected by spin_locks and also a
reference to the tcon should be acquired before using it safely.

Also you can extend SMB2_ioctl argument list to allow passing ipc_tid.
In this case SMB2_ioctl() can choose what to use - ipc_tid or tcon; if
tcon is passed -use it, otherwise - use ipc_tid.

In this case you can fallback to ipc_tid if no tcon is found in the list.

> +
> +       utf16_path = cifs_strndup_to_utf16(search_name, PATH_MAX,
> +                                          &utf16_path_len,
> +                                          nls_codepage, remap);
> +       if (!utf16_path) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +
> +       dfs_req_size = sizeof(*dfs_req) + utf16_path_len;
> +       dfs_req = kzalloc(dfs_req_size, GFP_KERNEL);
> +       if (!dfs_req) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +
> +       /* Highest DFS referral version understood */
> +       dfs_req->MaxReferralLevel = cpu_to_le16(DFS_VERSION);
> +
> +       /* Path to resolve in an UTF-16 null-terminated string */
> +       memcpy(dfs_req->RequestFileName, utf16_path, utf16_path_len);
> +
> +       do {
> +               rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> +                               FSCTL_DFS_GET_REFERRALS, true /* is_fsctl */,
> +                               (char *)dfs_req, dfs_req_size,
> +                               (char **)&dfs_rsp, &dfs_rsp_size);
> +       } while (rc == -EAGAIN);
> +
> +       if (rc) {
> +               cifs_dbg(VFS, "ioctl error in smb2_get_dfs_refer rc=%d\n", rc);
> +               goto out;
> +       }
> +
> +       rc = parse_dfs_referrals(dfs_rsp, dfs_rsp_size,
> +                                num_of_nodes, target_nodes,
> +                                nls_codepage, remap, search_name,
> +                                true /* is_unicode */);
> +       if (rc) {
> +               cifs_dbg(VFS, "parse error in smb2_get_dfs_refer rc=%d\n", rc);
> +               goto out;
> +       }
> +
> + out:
> +       kfree(utf16_path);
> +       kfree(dfs_req);
> +       kfree(dfs_rsp);
> +       return rc;
> +}
>  #define SMB2_SYMLINK_STRUCT_SIZE \
>         (sizeof(struct smb2_err_rsp) - 1 + sizeof(struct smb2_symlink_err_rsp))
>
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index c03b252..18700fd 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -695,6 +695,14 @@ struct fsctl_get_integrity_information_rsp {
>  /* Integrity flags for above */
>  #define FSCTL_INTEGRITY_FLAG_CHECKSUM_ENFORCEMENT_OFF  0x00000001
>
> +/* See MS-DFSC 2.2.2 */
> +struct fsctl_get_dfs_referral_req {
> +       __le16 MaxReferralLevel;
> +       __u8 RequestFileName[];
> +} __packed;
> +
> +/* DFS response is struct get_dfs_refer_rsp */
> +
>  /* See MS-SMB2 2.2.31.3 */
>  struct network_resiliency_req {
>         __le32 Timeout;
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aurelien Aptel Feb. 24, 2017, 3:25 p.m. UTC | #2
Hi Pavel,

Pavel Shilovsky <piastryyy@gmail.com> writes:
> Accessing the tcon_list should be protected by spin_locks and also a
> reference to the tcon should be acquired before using it safely.

I've noticed we have the cifs_tcp_ses_lock but also a per-session
session_mutex. I'm guessing we want cifs_tcp_ses_lock here.

By acquiring a reference you mean incrementing/decrementing tc_count. Do
we need to protect this inc/dec operation with cifs_tcp_ses_lock too?

> Also you can extend SMB2_ioctl argument list to allow passing ipc_tid.
> In this case SMB2_ioctl() can choose what to use - ipc_tid or tcon; if
> tcon is passed -use it, otherwise - use ipc_tid.
>
> In this case you can fallback to ipc_tid if no tcon is found in the list.
>

SMB2_ioctl() uses the tcon for many things. I don't see how we can just
use ipc_tid number if we don't pass a tcon.

Or do you mean: add a use_ipc bool to SMB2_ioctl arg list and use the tcon as
usual except overwrite the Tid field with ses->ipc_tid before sending?

Thanks,
Pavel Shilovsky Feb. 24, 2017, 7:50 p.m. UTC | #3
2017-02-24 7:25 GMT-08:00 Aurélien Aptel <aaptel@suse.com>:
> Hi Pavel,
>
> Pavel Shilovsky <piastryyy@gmail.com> writes:
>> Accessing the tcon_list should be protected by spin_locks and also a
>> reference to the tcon should be acquired before using it safely.
>
> I've noticed we have the cifs_tcp_ses_lock but also a per-session
> session_mutex. I'm guessing we want cifs_tcp_ses_lock here.
>
> By acquiring a reference you mean incrementing/decrementing tc_count. Do
> we need to protect this inc/dec operation with cifs_tcp_ses_lock too?

Yes.

>
>> Also you can extend SMB2_ioctl argument list to allow passing ipc_tid.
>> In this case SMB2_ioctl() can choose what to use - ipc_tid or tcon; if
>> tcon is passed -use it, otherwise - use ipc_tid.
>>
>> In this case you can fallback to ipc_tid if no tcon is found in the list.
>>
>
> SMB2_ioctl() uses the tcon for many things. I don't see how we can just
> use ipc_tid number if we don't pass a tcon.
>
> Or do you mean: add a use_ipc bool to SMB2_ioctl arg list and use the tcon as
> usual except overwrite the Tid field with ses->ipc_tid before sending?

We can pass both arguments: tcon and ipc_tid. If the 1st is specified
SMB_ioctl uses it, otherwise - use ipc_tid.

Since we call get_dfs_refer() once we get ipc_tid, may be it is worth
to try ipc_tid at first and then, if EAGAIN is returned (probably due
to reconnection that makes ipc_tid equal 0), use the 1st tcon from
list for the session. This will also workaround the bug in Samba.

>
> Thanks,
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
Aurelien Aptel Feb. 28, 2017, 6:44 p.m. UTC | #4
Aurélien Aptel <aaptel@suse.com> writes:
> What I'm proposing is to add a bool flag use_ipc to SMB2_ioctl. And do
> something like this:
>
>     if use_ipc:
>         if tcon->ses->ipc_tid == 0:
>             return -ENOTCONN
>         else:
>             req->tid = tcon->ses->ipc_tid
>
> Now in smb2_get_dfs_referral can first try to call it with
> use_ipc=true and if that fails with ENOTCONN try again with false.

I went ahead and implemented this along with small improvements in a v3
patchset. I'm still open to rework on this if can understand better what
you were proposing.

Cheers,
Pavel Shilovsky March 1, 2017, 2:34 a.m. UTC | #5
2017-02-27 6:17 GMT-08:00 Aurélien Aptel <aaptel@suse.com>:
> Pavel Shilovsky <piastryyy@gmail.com> writes:
>> We can pass both arguments: tcon and ipc_tid. If the 1st is specified
>> SMB_ioctl uses it, otherwise - use ipc_tid.
>
> I still don't get it. When tcon is not specified (I'm assuming that
> means passing NULL) and we just have ipc_tid (an int) we cannot:
> - access the struct cifs_ses*
> - access the struct TCP_Server_Info*
> - check if encryption is required
> - call any of the functions that requires the session or the server
>   struct (i.e. all the functions that do the real work).

Yes, we still need to pass "ses" argument which will create a whole
mess. But we probably need to clean this anyway because the number of
argument are getting too big.

>
>> Since we call get_dfs_refer() once we get ipc_tid, may be it is worth
>> to try ipc_tid at first and then, if EAGAIN is returned (probably due
>> to reconnection that makes ipc_tid equal 0), use the 1st tcon from
>> list for the session. This will also workaround the bug in Samba.
>
> What I'm proposing is to add a bool flag use_ipc to SMB2_ioctl. And do
> something like this:
>
>     if use_ipc:
>         if tcon->ses->ipc_tid == 0:
>             return -ENOTCONN
>         else:
>             req->tid = tcon->ses->ipc_tid
>
> Now in smb2_get_dfs_referral can first try to call it with
> use_ipc=true and if that fails with ENOTCONN try again with false.

This also works although it doesn't allow to call SMB2_ioctl() without
having at least one tcon.

Patch
diff mbox

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index a44b4db..2563fe8 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1097,6 +1097,78 @@  smb2_new_lease_key(struct cifs_fid *fid)
 	generate_random_uuid(fid->lease_key);
 }
 
+static int
+smb2_get_dfs_refer(const unsigned int xid, struct cifs_ses *ses,
+		   const char *search_name,
+		   struct dfs_info3_param **target_nodes,
+		   unsigned int *num_of_nodes,
+		   const struct nls_table *nls_codepage, int remap)
+{
+	int rc = -EINVAL;
+	__le16 *utf16_path = NULL;
+	int utf16_path_len = 0;
+	struct cifs_tcon *tcon;
+	struct fsctl_get_dfs_referral_req *dfs_req = NULL;
+	struct get_dfs_referral_rsp *dfs_rsp = NULL;
+	u32 dfs_req_size = 0, dfs_rsp_size = 0;
+
+	cifs_dbg(FYI, "smb2_get_dfs_refer path <%s>\n", search_name);
+
+	/*
+	 * Use any tcon from the current session. Here, the first one.
+	 */
+	tcon = list_first_entry(&ses->tcon_list, struct cifs_tcon, tcon_list);
+	if (!tcon)
+		goto out;
+
+	utf16_path = cifs_strndup_to_utf16(search_name, PATH_MAX,
+					   &utf16_path_len,
+					   nls_codepage, remap);
+	if (!utf16_path) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	dfs_req_size = sizeof(*dfs_req) + utf16_path_len;
+	dfs_req = kzalloc(dfs_req_size, GFP_KERNEL);
+	if (!dfs_req) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	/* Highest DFS referral version understood */
+	dfs_req->MaxReferralLevel = cpu_to_le16(DFS_VERSION);
+
+	/* Path to resolve in an UTF-16 null-terminated string */
+	memcpy(dfs_req->RequestFileName, utf16_path, utf16_path_len);
+
+	do {
+		rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+				FSCTL_DFS_GET_REFERRALS, true /* is_fsctl */,
+				(char *)dfs_req, dfs_req_size,
+				(char **)&dfs_rsp, &dfs_rsp_size);
+	} while (rc == -EAGAIN);
+
+	if (rc) {
+		cifs_dbg(VFS, "ioctl error in smb2_get_dfs_refer rc=%d\n", rc);
+		goto out;
+	}
+
+	rc = parse_dfs_referrals(dfs_rsp, dfs_rsp_size,
+				 num_of_nodes, target_nodes,
+				 nls_codepage, remap, search_name,
+				 true /* is_unicode */);
+	if (rc) {
+		cifs_dbg(VFS, "parse error in smb2_get_dfs_refer rc=%d\n", rc);
+		goto out;
+	}
+
+ out:
+	kfree(utf16_path);
+	kfree(dfs_req);
+	kfree(dfs_rsp);
+	return rc;
+}
 #define SMB2_SYMLINK_STRUCT_SIZE \
 	(sizeof(struct smb2_err_rsp) - 1 + sizeof(struct smb2_symlink_err_rsp))
 
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index c03b252..18700fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -695,6 +695,14 @@  struct fsctl_get_integrity_information_rsp {
 /* Integrity flags for above */
 #define FSCTL_INTEGRITY_FLAG_CHECKSUM_ENFORCEMENT_OFF	0x00000001
 
+/* See MS-DFSC 2.2.2 */
+struct fsctl_get_dfs_referral_req {
+	__le16 MaxReferralLevel;
+	__u8 RequestFileName[];
+} __packed;
+
+/* DFS response is struct get_dfs_refer_rsp */
+
 /* See MS-SMB2 2.2.31.3 */
 struct network_resiliency_req {
 	__le32 Timeout;