[linux-cifs-client] Re: [PATCH 3/7] cifs: don't declare smb_vol info on the stack

Steve French smfrench at gmail.com
Sun Nov 30 20:49:04 GMT 2008


I have thought about this a few times, but always hesitated because I
was hoping that eventually cifs mount option parsing could use new
parsing functions (which would also shrink the "parse_mount_options"
routine which we call during cifs mount), and would cause us to
rewrite this.  IIRC nfs was changed a few years ago to use the new
mount mechanism (match_table and tokens).

On Sun, Nov 30, 2008 at 12:40 PM, Jeff Layton <jlayton at redhat.com> wrote:
> struct smb_vol is fairly large, it's probably best to kzalloc it...
>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> ---
>  fs/cifs/connect.c |   91 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 49 insertions(+), 42 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 291ccd6..b08c6a3 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2186,27 +2186,30 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  {
>        int rc = 0;
>        int xid;
> -       struct smb_vol volume_info;
> +       struct smb_vol *volume_info;
>        struct cifsSesInfo *pSesInfo = NULL;
>        struct cifsTconInfo *tcon = NULL;
>        struct TCP_Server_Info *srvTcp = NULL;
>
>        xid = GetXid();
>
> -/* cFYI(1, ("Entering cifs_mount. Xid: %d with: %s", xid, mount_data)); */
> +       volume_info = kzalloc(sizeof(struct smb_vol), GFP_KERNEL);
> +       if (!volume_info) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
>
> -       memset(&volume_info, 0, sizeof(struct smb_vol));
> -       if (cifs_parse_mount_options(mount_data, devname, &volume_info)) {
> +       if (cifs_parse_mount_options(mount_data, devname, volume_info)) {
>                rc = -EINVAL;
>                goto out;
>        }
>
> -       if (volume_info.nullauth) {
> +       if (volume_info->nullauth) {
>                cFYI(1, ("null user"));
> -               volume_info.username = "";
> -       } else if (volume_info.username) {
> +               volume_info->username = "";
> +       } else if (volume_info->username) {
>                /* BB fixme parse for domain name here */
> -               cFYI(1, ("Username: %s", volume_info.username));
> +               cFYI(1, ("Username: %s", volume_info->username));
>        } else {
>                cifserror("No username specified");
>        /* In userspace mount helper we can get user name from alternate
> @@ -2217,27 +2220,27 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>
>
>        /* this is needed for ASCII cp to Unicode converts */
> -       if (volume_info.iocharset == NULL) {
> +       if (volume_info->iocharset == NULL) {
>                cifs_sb->local_nls = load_nls_default();
>        /* load_nls_default can not return null */
>        } else {
> -               cifs_sb->local_nls = load_nls(volume_info.iocharset);
> +               cifs_sb->local_nls = load_nls(volume_info->iocharset);
>                if (cifs_sb->local_nls == NULL) {
>                        cERROR(1, ("CIFS mount error: iocharset %s not found",
> -                                volume_info.iocharset));
> +                                volume_info->iocharset));
>                        rc = -ELIBACC;
>                        goto out;
>                }
>        }
>
>        /* get a reference to a tcp session */
> -       srvTcp = cifs_get_tcp_session(&volume_info);
> +       srvTcp = cifs_get_tcp_session(volume_info);
>        if (IS_ERR(srvTcp)) {
>                rc = PTR_ERR(srvTcp);
>                goto out;
>        }
>
> -       pSesInfo = cifs_find_smb_ses(srvTcp, volume_info.username);
> +       pSesInfo = cifs_find_smb_ses(srvTcp, volume_info->username);
>        if (pSesInfo) {
>                cFYI(1, ("Existing smb sess found (status=%d)",
>                        pSesInfo->status));
> @@ -2275,24 +2278,24 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                list_add(&pSesInfo->smb_ses_list, &srvTcp->smb_ses_list);
>                write_unlock(&cifs_tcp_ses_lock);
>
> -               /* volume_info.password freed at unmount */
> -               if (volume_info.password) {
> -                       pSesInfo->password = volume_info.password;
> +               /* volume_info->password freed at unmount */
> +               if (volume_info->password) {
> +                       pSesInfo->password = volume_info->password;
>                        /* set to NULL to prevent freeing on exit */
> -                       volume_info.password = NULL;
> +                       volume_info->password = NULL;
>                }
> -               if (volume_info.username)
> -                       strncpy(pSesInfo->userName, volume_info.username,
> +               if (volume_info->username)
> +                       strncpy(pSesInfo->userName, volume_info->username,
>                                MAX_USERNAME_SIZE);
> -               if (volume_info.domainname) {
> -                       int len = strlen(volume_info.domainname);
> +               if (volume_info->domainname) {
> +                       int len = strlen(volume_info->domainname);
>                        pSesInfo->domainName = kmalloc(len + 1, GFP_KERNEL);
>                        if (pSesInfo->domainName)
>                                strcpy(pSesInfo->domainName,
> -                                       volume_info.domainname);
> +                                       volume_info->domainname);
>                }
> -               pSesInfo->linux_uid = volume_info.linux_uid;
> -               pSesInfo->overrideSecFlg = volume_info.secFlg;
> +               pSesInfo->linux_uid = volume_info->linux_uid;
> +               pSesInfo->overrideSecFlg = volume_info->secFlg;
>                down(&pSesInfo->sesSem);
>
>                /* BB FIXME need to pass vol->secFlgs BB */
> @@ -2303,14 +2306,14 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>
>        /* search for existing tcon to this server share */
>        if (!rc) {
> -               setup_cifs_sb(&volume_info, cifs_sb);
> +               setup_cifs_sb(volume_info, cifs_sb);
>
> -               tcon = cifs_find_tcon(pSesInfo, volume_info.UNC);
> +               tcon = cifs_find_tcon(pSesInfo, volume_info->UNC);
>                if (tcon) {
>                        cFYI(1, ("Found match on UNC path"));
>                        /* existing tcon already has a reference */
>                        cifs_put_smb_ses(pSesInfo);
> -                       if (tcon->seal != volume_info.seal)
> +                       if (tcon->seal != volume_info->seal)
>                                cERROR(1, ("transport encryption setting "
>                                           "conflicts with existing tid"));
>                } else {
> @@ -2322,8 +2325,8 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                        tcon->ses = pSesInfo;
>
>                        /* check for null share name ie connect to dfs root */
> -                       if ((strchr(volume_info.UNC + 3, '\\') == NULL)
> -                           && (strchr(volume_info.UNC + 3, '/') == NULL)) {
> +                       if ((strchr(volume_info->UNC + 3, '\\') == NULL)
> +                           && (strchr(volume_info->UNC + 3, '/') == NULL)) {
>                                /* rc = connect_to_dfs_path(...) */
>                                cFYI(1, ("DFS root not supported"));
>                                rc = -ENODEV;
> @@ -2332,10 +2335,10 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                                /* BB Do we need to wrap sesSem around
>                                 * this TCon call and Unix SetFS as
>                                 * we do on SessSetup and reconnect? */
> -                               rc = CIFSTCon(xid, pSesInfo, volume_info.UNC,
> +                               rc = CIFSTCon(xid, pSesInfo, volume_info->UNC,
>                                              tcon, cifs_sb->local_nls);
>                                cFYI(1, ("CIFS Tcon rc = %d", rc));
> -                               if (volume_info.nodfs) {
> +                               if (volume_info->nodfs) {
>                                        tcon->Flags &= ~SMB_SHARE_IS_IN_DFS;
>                                        cFYI(1, ("DFS disabled (%d)",
>                                                tcon->Flags));
> @@ -2343,7 +2346,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                        }
>                        if (rc)
>                                goto mount_fail_check;
> -                       tcon->seal = volume_info.seal;
> +                       tcon->seal = volume_info->seal;
>                        write_lock(&cifs_tcp_ses_lock);
>                        list_add(&tcon->tcon_list, &pSesInfo->tcon_list);
>                        write_unlock(&cifs_tcp_ses_lock);
> @@ -2353,9 +2356,9 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                   to a share so for resources mounted more than once
>                   to the same server share the last value passed in
>                   for the retry flag is used */
> -               tcon->retry = volume_info.retry;
> -               tcon->nocase = volume_info.nocase;
> -               tcon->local_lease = volume_info.local_lease;
> +               tcon->retry = volume_info->retry;
> +               tcon->nocase = volume_info->nocase;
> +               tcon->local_lease = volume_info->local_lease;
>        }
>        if (pSesInfo) {
>                if (pSesInfo->capabilities & CAP_LARGE_FILES) {
> @@ -2392,7 +2395,7 @@ mount_fail_check:
>        if (tcon->ses->capabilities & CAP_UNIX)
>                /* reset of caps checks mount to see if unix extensions
>                   disabled for just this mount */
> -               reset_cifs_unix_caps(xid, tcon, sb, &volume_info);
> +               reset_cifs_unix_caps(xid, tcon, sb, volume_info);
>        else
>                tcon->unix_ext = 0; /* server does not support them */
>
> @@ -2411,18 +2414,22 @@ mount_fail_check:
>                cifs_sb->rsize = min(cifs_sb->rsize,
>                               (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
>
> -       /* volume_info.password is freed above when existing session found
> +       /* volume_info->password is freed above when existing session found
>        (in which case it is not needed anymore) but when new sesion is created
>        the password ptr is put in the new session structure (in which case the
>        password will be freed at unmount time) */
>  out:
>        /* zero out password before freeing */
> -       if (volume_info.password != NULL) {
> -               memset(volume_info.password, 0, strlen(volume_info.password));
> -               kfree(volume_info.password);
> +       if (volume_info) {
> +               if (volume_info->password != NULL) {
> +                       memset(volume_info->password, 0,
> +                               strlen(volume_info->password));
> +                       kfree(volume_info->password);
> +               }
> +               kfree(volume_info->UNC);
> +               kfree(volume_info->prepath);
> +               kfree(volume_info);
>        }
> -       kfree(volume_info.UNC);
> -       kfree(volume_info.prepath);
>        FreeXid(xid);
>        return rc;
>  }
> --
> 1.5.5.1
>
>



-- 
Thanks,

Steve


More information about the linux-cifs-client mailing list