[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-cifs
Subject:    Re: [PATCH] cifs: prevent infinite recursion in cifs_reconnect_tcon
From:       Steve French <smfrench () gmail ! com>
Date:       2010-09-29 20:11:52
Message-ID: AANLkTi=o3ZgrLgwzi65e1mfut45uGmLNpSyYZ2=qFTZj () mail ! gmail ! com
[Download RAW message or body]

On Wed, Sep 29, 2010 at 3:10 PM, Shirish Pargaonkar
<shirishpargaonkar@gmail.com> wrote:
> On Wed, Sep 29, 2010 at 2:27 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> cifs_reconnect_tcon is called from smb_init. After a successful
>> reconnect, cifs_reconnect_tcon will call reset_cifs_unix_caps. That
>> function will, in turn call CIFSSMBQFSUnixInfo and CIFSSMBSetFSUnixInfo.
>> Those functions also call smb_init.
>>
>> It's possible for the session and tcon reconnect to succeed, and then
>> for another cifs_reconnect to occur before CIFSSMBQFSUnixInfo or
>> CIFSSMBSetFSUnixInfo to be called. That'll cause those functions to call
>> smb_init and cifs_reconnect_tcon again, ad infinitum...
>>
>> Break the infinite recursion by having those functions use a new
>> smb_init variant that doesn't attempt to perform a reconnect.
>>
>> Reported-and-Tested-by: Michal Suchanek <hramrach@centrum.cz>
>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> ---
>>  fs/cifs/cifssmb.c |   49 +++++++++++++++++++++++++++++++++----------------
>>  1 files changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 13c854e..54bd83a 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -232,7 +232,7 @@ static int
>>  small_smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>>                void **request_buf)
>>  {
>> -       int rc = 0;
>> +       int rc;
>>
>>        rc = cifs_reconnect_tcon(tcon, smb_command);
>>        if (rc)
>> @@ -250,7 +250,7 @@ small_smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>>        if (tcon != NULL)
>>                cifs_stats_inc(&tcon->num_smbs_sent);
>>
>> -       return rc;
>> +       return 0;
>>  }
>>
>>  int
>> @@ -281,16 +281,9 @@ small_smb_init_no_tc(const int smb_command, const int wct,
>>
>>  /* If the return code is zero, this function must fill in request_buf pointer */
>>  static int
>> -smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>> -        void **request_buf /* returned */ ,
>> -        void **response_buf /* returned */ )
>> +__smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>> +                       void **request_buf, void **response_buf)
>>  {
>> -       int rc = 0;
>> -
>> -       rc = cifs_reconnect_tcon(tcon, smb_command);
>> -       if (rc)
>> -               return rc;
>> -
>>        *request_buf = cifs_buf_get();
>>        if (*request_buf == NULL) {
>>                /* BB should we add a retry in here if not a writepage? */
>> @@ -309,7 +302,31 @@ smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>>        if (tcon != NULL)
>>                cifs_stats_inc(&tcon->num_smbs_sent);
>>
>> -       return rc;
>> +       return 0;
>> +}
>> +
>> +/* If the return code is zero, this function must fill in request_buf pointer */
>> +static int
>> +smb_init(int smb_command, int wct, struct cifsTconInfo *tcon,
>> +        void **request_buf, void **response_buf)
>> +{
>> +       int rc;
>> +
>> +       rc = cifs_reconnect_tcon(tcon, smb_command);
>> +       if (rc)
>> +               return rc;
>> +
>> +       return __smb_init(smb_command, wct, tcon, request_buf, response_buf);
>> +}
>> +
>> +static int
>> +smb_init_no_reconnect(int smb_command, int wct, struct cifsTconInfo *tcon,
>> +                       void **request_buf, void **response_buf)
>> +{
>> +       if (tcon->ses->need_reconnect || tcon->need_reconnect)
>> +               return -EHOSTDOWN;
>> +
>> +       return __smb_init(smb_command, wct, tcon, request_buf, response_buf);
>>  }
>>
>>  static int validate_t2(struct smb_t2_rsp *pSMB)
>> @@ -4536,8 +4553,8 @@ CIFSSMBQFSUnixInfo(const int xid, struct cifsTconInfo *tcon)
>>
>>        cFYI(1, "In QFSUnixInfo");
>>  QFSUnixRetry:
>> -       rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
>> -                     (void **) &pSMBr);
>> +       rc = smb_init_no_reconnect(SMB_COM_TRANSACTION2, 15, tcon,
>> +                                  (void **) &pSMB, (void **) &pSMBr);
>>        if (rc)
>>                return rc;
>>
>> @@ -4606,8 +4623,8 @@ CIFSSMBSetFSUnixInfo(const int xid, struct cifsTconInfo *tcon, __u64 cap)
>>        cFYI(1, "In SETFSUnixInfo");
>>  SETFSUnixRetry:
>>        /* BB switch to small buf init to save memory */
>> -       rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
>> -                     (void **) &pSMBr);
>> +       rc = smb_init_no_reconnect(SMB_COM_TRANSACTION2, 15, tcon,
>> +                                       (void **) &pSMB, (void **) &pSMBr);
>>        if (rc)
>>                return rc;
>>
>> --
>> 1.7.2.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
>>
>
> Jeff, is same code change is needed in non-unix versions of these functions
> such as CIFSSMBQFSInfo?

No - reset_cifs_unix_caps is called in reconnect from smb_init (thus
can recurse) but the others are not



-- 
Thanks,

Steve
--
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
[prev in list] [next in list] [prev in thread] [next in thread]