[v1,2/3] CIFS: make IPC a regular tcon

* Remove ses->ipc_tid.
* Make IPC$ regular tcon by having it stored in the session tcon list.
* Add a direct pointer to it in ses->tcon_ipc.
* Distinguish PIPE tcon from IPC tcon by adding a tcon->pipe flag. All
  IPC tcons are pipes but not all pipes are IPC.
* All TreeConnect functions now cannot take a NULL tcon object.

The IPC tcon has the same lifetime as the session it belongs to. It is
created when the session is created and destroyed when the session is
destroyed.

Since no mounts directly refer to the IPC tcon, its refcount should
always be 0. Thus we make sure cifs_put_tcon() skips it.

If the mount request resulting in a new session being created requires
encryption, try to require it too for IPC.

NOTE TO BACKPORTER: the previous SERVER_NAME_LENGTH change MUST be
included with this.

Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/cifsglob.h |   7 ++-
 fs/cifs/cifssmb.c  |   5 +-
 fs/cifs/connect.c  | 159 ++++++++++++++++++++++++++++++++++++++++-------------
 fs/cifs/inode.c    |   2 +-
 fs/cifs/smb2pdu.c  |  32 ++---------
 5 files changed, 134 insertions(+), 71 deletions(-)

2018-01-17 9:21 GMT-08:00 Aurelien Aptel <aaptel@suse.com>:
> * Remove ses->ipc_tid.
> * Make IPC$ regular tcon by having it stored in the session tcon list.

Thank you for doing this, I suspected that we would end up with something like it anyway.

> * Add a direct pointer to it in ses->tcon_ipc.
> * Distinguish PIPE tcon from IPC tcon by adding a tcon->pipe flag. All
>   IPC tcons are pipes but not all pipes are IPC.
> * All TreeConnect functions now cannot take a NULL tcon object.
>
> The IPC tcon has the same lifetime as the session it belongs to. It is
> created when the session is created and destroyed when the session is
> destroyed.
>
> Since no mounts directly refer to the IPC tcon, its refcount should
> always be 0. Thus we make sure cifs_put_tcon() skips it.
>
> If the mount request resulting in a new session being created requires
> encryption, try to require it too for IPC.
>
> NOTE TO BACKPORTER: the previous SERVER_NAME_LENGTH change MUST be
> included with this.

The 1st patch is small, so let's merge both patches that would make backporter's life easier.

>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/cifsglob.h |   7 ++-
>  fs/cifs/cifssmb.c  |   5 +-
>  fs/cifs/connect.c  | 159 ++++++++++++++++++++++++++++++++++++++++-------------
>  fs/cifs/inode.c    |   2 +-
>  fs/cifs/smb2pdu.c  |  32 ++---------
>  5 files changed, 134 insertions(+), 71 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index eca386d8dcfb..804ec79af5e1 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -822,12 +822,12 @@ static inline void cifs_set_net_ns(struct TCP_Server_Info *srv, struct net *net)
>  struct cifs_ses {
>         struct list_head smb_ses_list;
>         struct list_head tcon_list;
> +       struct cifs_tcon *tcon_ipc;
>         struct mutex session_mutex;
>         struct TCP_Server_Info *server; /* pointer to server info */
>         int ses_count;          /* reference counter */
>         enum statusEnum status;
>         unsigned overrideSecFlg;  /* if non-zero override global sec flags */
> -       __u32 ipc_tid;          /* special tid for connection to IPC share */
>         char *serverOS;         /* name of operating system underlying server */
>         char *serverNOS;        /* name of network operating system of server */
>         char *serverDomain;     /* security realm of server */
> @@ -930,7 +930,9 @@ struct cifs_tcon {
>         FILE_SYSTEM_DEVICE_INFO fsDevInfo;
>         FILE_SYSTEM_ATTRIBUTE_INFO fsAttrInfo; /* ok if fs name truncated */
>         FILE_SYSTEM_UNIX_INFO fsUnixInfo;
> -       bool ipc:1;             /* set if connection to IPC$ eg for RPC/PIPES */
> +       bool ipc:1;   /* set if connection to IPC$ share (always also pipe) */
> +       bool pipe:1;  /* set if connection to pipe share */
> +       bool print:1; /* set if connection to printer share */
>         bool retry:1;
>         bool nocase:1;
>         bool seal:1;      /* transport encryption for this mounted share */
> @@ -943,7 +945,6 @@ struct cifs_tcon {
>         bool need_reopen_files:1; /* need to reopen tcon file handles */
>         bool use_resilient:1; /* use resilient instead of durable handles */
>         bool use_persistent:1; /* use persistent instead of durable handles */
> -       bool print:1;           /* set if connection to printer share */
>         __le32 capabilities;
>         __u32 share_flags;
>         __u32 maximal_access;
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 35dc5bf01ee2..2f8dea905ef9 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -4822,8 +4822,9 @@ CIFSGetDFSRefer(const unsigned int xid, struct cifs_ses *ses,
>         *target_nodes = NULL;
>
>         cifs_dbg(FYI, "In GetDFSRefer the path %s\n", search_name);
> -       if (ses == NULL)
> +       if (ses == NULL || ses->tcon_ipc == NULL)
>                 return -ENODEV;
> +
>  getDFSRetry:
>         rc = smb_init(SMB_COM_TRANSACTION2, 15, NULL, (void **) &pSMB,
>                       (void **) &pSMBr);
> @@ -4833,7 +4834,7 @@ CIFSGetDFSRefer(const unsigned int xid, struct cifs_ses *ses,
>         /* server pointer checked in called function,
>         but should never be null here anyway */
>         pSMB->hdr.Mid = get_next_mid(ses->server);
> -       pSMB->hdr.Tid = ses->ipc_tid;
> +       pSMB->hdr.Tid = ses->tcon_ipc->tid;
>         pSMB->hdr.Uid = ses->Suid;
>         if (ses->capabilities & CAP_STATUS32)
>                 pSMB->hdr.Flags2 |= SMBFLG2_ERR_STATUS;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 0bfc2280436d..5ae393fe0c5d 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -353,7 +353,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>         list_for_each(tmp, &server->smb_ses_list) {
>                 ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
>                 ses->need_reconnect = true;
> -               ses->ipc_tid = 0;
>                 list_for_each(tmp2, &ses->tcon_list) {
>                         tcon = list_entry(tmp2, struct cifs_tcon, tcon_list);
>                         tcon->need_reconnect = true;
> @@ -2381,6 +2380,104 @@ static int match_session(struct cifs_ses *ses, struct smb_vol *vol)
>         return 1;
>  }
>
> +/**
> + * cifs_setup_ipc - helper to setup the IPC tcon for the session
> + *
> + * A new IPC connection is made and added to the session tcon list
> + * (also accessible through ses->tcon_ipc). The IPC tcon has the same
> + * lifetime as the session.
> + */
> +static int
> +cifs_setup_ipc(struct cifs_ses *ses, struct smb_vol *volume_info)
> +{
> +       int rc = 0, xid;
> +       struct cifs_tcon *tcon;
> +       struct nls_table *nls_codepage;
> +       char unc[SERVER_NAME_LENGTH + sizeof("//x/IPC$")] = {0};
> +       bool seal = false;
> +
> +       /*
> +        * If the mount request that resulted in the creation of the
> +        * session requires encryption, force IPC to be encrypted too.
> +        */
> +       if (volume_info->seal) {
> +               if (ses->server->capabilities & SMB2_GLOBAL_CAP_ENCRYPTION)
> +                       seal = true;
> +               else {
> +                       cifs_dbg(VFS,
> +                                "IPC: server doesn't support encryption\n");
> +                       return -EOPNOTSUPP;
> +               }
> +       }
> +
> +       tcon = tconInfoAlloc();
> +       if (tcon == NULL)
> +               return -ENOMEM;
> +
> +       snprintf(unc, sizeof(unc), "\\\\%s\\IPC$", ses->serverName);
> +
> +       /* cannot fail */
> +       nls_codepage = load_nls_default();
> +
> +       xid = get_xid();
> +       tcon->ses = ses;
> +       tcon->ipc = true;
> +       tcon->seal = seal;
> +       rc = ses->server->ops->tree_connect(xid, ses, unc, tcon, nls_codepage);
> +       free_xid(xid);
> +
> +       if (rc) {
> +               cifs_dbg(VFS, "failed to connect to IPC (rc=%d)\n", rc);
> +               tconInfoFree(tcon);
> +               goto out;
> +       }
> +
> +       cifs_dbg(FYI, "IPC tcon rc = %d ipc tid = %d\n", rc, tcon->tid);
> +
> +       spin_lock(&cifs_tcp_ses_lock);
> +       list_add(&tcon->tcon_list, &ses->tcon_list);
> +       ses->tcon_ipc = tcon;
> +       spin_unlock(&cifs_tcp_ses_lock);
> +
> +out:
> +       unload_nls(nls_codepage);
> +       return rc;
> +}
> +
> +/**
> + * cifs_free_ipc - helper to release the session IPC tcon
> + *
> + * Needs to be called everytime a session is destroyed
> + */
> +static int
> +cifs_free_ipc(struct cifs_ses *ses)
> +{
> +       int rc = 0, xid;
> +       struct cifs_tcon *tcon;
> +
> +       spin_lock(&cifs_tcp_ses_lock);
> +       tcon = ses->tcon_ipc;
> +       if (tcon == NULL) {
> +               spin_unlock(&cifs_tcp_ses_lock);
> +               return 0;
> +       }
> +       list_del_init(&tcon->tcon_list);
> +       spin_unlock(&cifs_tcp_ses_lock);
> +
> +       if (ses->server->ops->tree_disconnect) {
> +               xid = get_xid();
> +               rc = ses->server->ops->tree_disconnect(xid, tcon);
> +               free_xid(xid);
> +       }
> +
> +       if (rc)
> +               cifs_dbg(FYI, "failed to disconnect IPC tcon (rc=%d)\n", rc);
> +
> +       tconInfoFree(tcon);
> +       ses->tcon_ipc = NULL;

Minor: it should probably work this way, but since we set ses->tcon_ipc under a spinlock in cifs_setup_ipc(), it would be better to do something similar here.

> +       return rc;
> +}
> +
>  static struct cifs_ses *
>  cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
>  {
> @@ -2421,6 +2518,8 @@ cifs_put_smb_ses(struct cifs_ses *ses)
>                 ses->status = CifsExiting;
>         spin_unlock(&cifs_tcp_ses_lock);
>
> +       cifs_free_ipc(ses);
> +
>         if (ses->status == CifsExiting && server->ops->logoff) {
>                 xid = get_xid();
>                 rc = server->ops->logoff(xid, ses);
> @@ -2665,6 +2764,9 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>         spin_unlock(&cifs_tcp_ses_lock);
>
>         free_xid(xid);
> +
> +       cifs_setup_ipc(ses, volume_info);
> +
>         return ses;
>
>  get_ses_fail:
> @@ -2709,8 +2811,16 @@ void
>  cifs_put_tcon(struct cifs_tcon *tcon)
>  {
>         unsigned int xid;
> -       struct cifs_ses *ses = tcon->ses;
> +       struct cifs_ses *ses;
> +
> +       /*
> +        * IPC tcon share the lifetime of their session and are
> +        * destroyed in the session put function
> +        */
> +       if (tcon == NULL || tcon->ipc)
> +               return;
>
> +       ses = tcon->ses;
>         cifs_dbg(FYI, "%s: tc_count=%d\n", __func__, tcon->tc_count);
>         spin_lock(&cifs_tcp_ses_lock);
>         if (--tcon->tc_count > 0) {
> @@ -2986,39 +3096,17 @@ get_dfs_path(const unsigned int xid, struct cifs_ses *ses, const char *old_path,
>              const struct nls_table *nls_codepage, unsigned int *num_referrals,
>              struct dfs_info3_param **referrals, int remap)
>  {
> -       char *temp_unc;
>         int rc = 0;
>
> -       if (!ses->server->ops->tree_connect || !ses->server->ops->get_dfs_refer)
> +       if (!ses->server->ops->get_dfs_refer)
>                 return -ENOSYS;
>
>         *num_referrals = 0;
>         *referrals = NULL;
>
> -       if (ses->ipc_tid == 0) {
> -               temp_unc = kmalloc(2 /* for slashes */ +
> -                       strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2)
> -                               + 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL);
> -               if (temp_unc == NULL)
> -                       return -ENOMEM;
> -               temp_unc[0] = '\\';
> -               temp_unc[1] = '\\';
> -               strcpy(temp_unc + 2, ses->serverName);
> -               strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$");
> -               rc = ses->server->ops->tree_connect(xid, ses, temp_unc, NULL,
> -                                                   nls_codepage);
> -               cifs_dbg(FYI, "Tcon rc = %d ipc_tid = %d\n", rc, ses->ipc_tid);
> -               kfree(temp_unc);
> -       }
> -       if (rc == 0)
> -               rc = ses->server->ops->get_dfs_refer(xid, ses, old_path,
> -                                                    referrals, num_referrals,
> -                                                    nls_codepage, remap);
> -       /*
> -        * BB - map targetUNCs to dfs_info3 structures, here or in
> -        * ses->server->ops->get_dfs_refer.
> -        */

Is this no longer needed?

> -
> +       rc = ses->server->ops->get_dfs_refer(xid, ses, old_path,
> +                                            referrals, num_referrals,
> +                                            nls_codepage, remap);
>         return rc;
>  }
>
> @@ -3783,7 +3871,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
>                 tcon->unix_ext = 0; /* server does not support them */
>
>         /* do not care if a following call succeed - informational */
> -       if (!tcon->ipc && server->ops->qfs_tcon)
> +       if (!tcon->pipe && server->ops->qfs_tcon)
>                 server->ops->qfs_tcon(xid, tcon);
>
>         cifs_sb->wsize = server->ops->negotiate_wsize(tcon, volume_info);
> @@ -3913,8 +4001,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
>  }
>
>  /*
> - * Issue a TREE_CONNECT request. Note that for IPC$ shares, that the tcon
> - * pointer may be NULL.
> + * Issue a TREE_CONNECT request.
>   */
>  int
>  CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
> @@ -3950,7 +4037,7 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>         pSMB->AndXCommand = 0xFF;
>         pSMB->Flags = cpu_to_le16(TCON_EXTENDED_SECINFO);
>         bcc_ptr = &pSMB->Password[0];
> -       if (!tcon || (ses->server->sec_mode & SECMODE_USER)) {
> +       if (tcon->pipe || (ses->server->sec_mode & SECMODE_USER)) {
>                 pSMB->PasswordLength = cpu_to_le16(1);  /* minimum */
>                 *bcc_ptr = 0; /* password is null byte */
>                 bcc_ptr++;              /* skip password */
> @@ -4022,7 +4109,7 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>                          0);
>
>         /* above now done in SendReceive */
> -       if ((rc == 0) && (tcon != NULL)) {
> +       if (rc == 0) {
>                 bool is_unicode;
>
>                 tcon->tidStatus = CifsGood;
> @@ -4042,7 +4129,8 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>                         if ((bcc_ptr[0] == 'I') && (bcc_ptr[1] == 'P') &&
>                             (bcc_ptr[2] == 'C')) {
>                                 cifs_dbg(FYI, "IPC connection\n");
> -                               tcon->ipc = 1;
> +                               tcon->ipc = true;
> +                               tcon->pipe = true;
>                         }
>                 } else if (length == 2) {
>                         if ((bcc_ptr[0] == 'A') && (bcc_ptr[1] == ':')) {
> @@ -4069,9 +4157,6 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>                 else
>                         tcon->Flags = 0;
>                 cifs_dbg(FYI, "Tcon flags: 0x%x\n", tcon->Flags);
> -       } else if ((rc == 0) && tcon == NULL) {
> -               /* all we need to save for IPC$ connection */
> -               ses->ipc_tid = smb_buffer_response->Tid;
>         }
>
>         cifs_buf_release(smb_buffer);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ecb99079363a..8f9a8cc7cc62 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1049,7 +1049,7 @@ struct inode *cifs_root_iget(struct super_block *sb)
>         tcon->resource_id = CIFS_I(inode)->uniqueid;
>  #endif
>
> -       if (rc && tcon->ipc) {
> +       if (rc && tcon->pipe) {
>                 cifs_dbg(FYI, "ipc connection - fake read inode\n");
>                 spin_lock(&inode->i_lock);
>                 inode->i_mode |= S_IFDIR;
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 01346b8b6edb..a5c94f8308eb 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1283,8 +1283,7 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>         }
>
>         /* SMB2 TREE_CONNECT request must be called with TreeId == 0 */
> -       if (tcon)
> -               tcon->tid = 0;
> +       tcon->tid = 0;
>
>         rc = small_smb2_init(SMB2_TREE_CONNECT, tcon, (void **) &req);
>         if (rc) {
> @@ -1292,15 +1291,7 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>                 return rc;
>         }
>
> -       if (tcon == NULL) {
> -               if ((ses->session_flags & SMB2_SESSION_FLAG_ENCRYPT_DATA))
> -                       flags |= CIFS_TRANSFORM_REQ;
> -
> -               /* since no tcon, smb2_init can not do this, so do here */
> -               req->hdr.sync_hdr.SessionId = ses->Suid;
> -               if (ses->server->sign)
> -                       req->hdr.sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> -       } else if (encryption_required(tcon))
> +       if (encryption_required(tcon))
>                 flags |= CIFS_TRANSFORM_REQ;
>
>         iov[0].iov_base = (char *)req;
> @@ -1328,21 +1319,16 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>                 goto tcon_error_exit;
>         }
>
> -       if (tcon == NULL) {
> -               ses->ipc_tid = rsp->hdr.sync_hdr.TreeId;
> -               goto tcon_exit;
> -       }
> -
>         switch (rsp->ShareType) {
>         case SMB2_SHARE_TYPE_DISK:
>                 cifs_dbg(FYI, "connection to disk share\n");
>                 break;
>         case SMB2_SHARE_TYPE_PIPE:
> -               tcon->ipc = true;
> +               tcon->pipe = true;
>                 cifs_dbg(FYI, "connection to pipe share\n");
>                 break;
>         case SMB2_SHARE_TYPE_PRINT:
> -               tcon->ipc = true;
> +               tcon->print = true;
>                 cifs_dbg(FYI, "connection to printer\n");
>                 break;
>         default:
> @@ -1913,16 +1899,6 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
>         if (rc)
>                 return rc;
>
> -       if (use_ipc) {
> -               if (ses->ipc_tid == 0) {
> -                       cifs_small_buf_release(req);
> -                       return -ENOTCONN;
> -               }
> -
> -               cifs_dbg(FYI, "replacing tid 0x%x with IPC tid 0x%x\n",
> -                        req->hdr.sync_hdr.TreeId, ses->ipc_tid);
> -               req->hdr.sync_hdr.TreeId = ses->ipc_tid;
> -       }
>         if (encryption_required(tcon))
>                 flags |= CIFS_TRANSFORM_REQ;
>
> --
> 2.12.3
>
> --
> 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


--
Best regards,
Pavel Shilovsky
--
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

Pavel Shilovskiy <pshilov@microsoft.com> writes:
> Minor: it should probably work this way, but since we set ses->tcon_ipc under a spinlock in cifs_setup_ipc(), it would be better to do something similar here.

I was actually thinking that the spinlock might not be necessary for
that pointer. It's always only set at session creation (new tcon) and
destruction (to null). In between, it's only going to be read. What do
you think?

>> -       /*
>> -        * BB - map targetUNCs to dfs_info3 structures, here or in
>> -        * ses->server->ops->get_dfs_refer.
>> -        */
>
> Is this no longer needed?

If you are talking about the tcon creation, it is now done all the time
at session creation. If you are talking about that specific comment, I
believe it's already done now in parse_dfs_referrals(). That function
parses the packet into dfs_info3_param structures.

2018-01-18 2:30 GMT-08:00 Aurélien Aptel <aaptel@suse.com>:
> Pavel Shilovskiy <pshilov@microsoft.com> writes:
>> Minor: it should probably work this way, but since we set ses->tcon_ipc under a spinlock in cifs_setup_ipc(), it would be better to do something similar here.
>
> I was actually thinking that the spinlock might not be necessary for
> that pointer. It's always only set at session creation (new tcon) and
> destruction (to null). In between, it's only going to be read. What do
> you think?

I agree that accessing ses->tcon_ipc is not necessary to be under
spinlock. Btw, why should we put this tcon to the list at the 1st
place? We can leave to be accessed only by ses->tcon_ipc and do not
bother with spinlocks at all.

>
>>> -       /*
>>> -        * BB - map targetUNCs to dfs_info3 structures, here or in
>>> -        * ses->server->ops->get_dfs_refer.
>>> -        */
>>
>> Is this no longer needed?
>
> If you are talking about the tcon creation, it is now done all the time
> at session creation. If you are talking about that specific comment, I
> believe it's already done now in parse_dfs_referrals(). That function
> parses the packet into dfs_info3_param structures.
>

I was talking about the comment.


--
Best regards,
Pavel Shilovsky
--
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

Pavel Shilovsky <piastryyy@gmail.com> writes:
> I agree that accessing ses->tcon_ipc is not necessary to be under
> spinlock. Btw, why should we put this tcon to the list at the 1st
> place? We can leave to be accessed only by ses->tcon_ipc and do not
> bother with spinlocks at all.

I thought about it. Initially I thought I could reuse tcon get/put
functions but since its not the case (no mount points refer to the IPC
tcon so it's special) I agree it would make more sense now.