| Message ID | 20170223144334.22320-4-aaptel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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,
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)
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,
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.
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;
Signed-off-by: Aurelien Aptel <aaptel@suse.com> --- fs/cifs/smb2ops.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/cifs/smb2pdu.h | 8 +++++++ 2 files changed, 80 insertions(+)