Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAH2r5msOSLaZT42-jFMjJrB1YiYTZBzdM18ieqQY2v=YwXzcrA@mail.gmail.com>
 <1241844c-c9ab-2055-a363-80db63a4dd22@talpey.com>
In-Reply-To: <1241844c-c9ab-2055-a363-80db63a4dd22@talpey.com>
From:   Steve French <smfrench@gmail.com>
Date:   Tue, 22 Jun 2021 12:09:28 -0500
Message-ID: <CAH2r5muyH4HSWcYZSZMD_TNtF-Gw4Z13FvQ7H9NmmqWetYKZUQ@mail.gmail.com>
Subject: Re: [SMBDIRECT][PATCH] missing rc checks while waiting for SMB3 over
 RDMA events
To:     Tom Talpey <tom@talpey.com>
Cc:     Long Li <longli@microsoft.com>, CIFS <linux-cifs@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: multipart/mixed; boundary="000000000000fe696805c55dda18"
Precedence: bulk

--000000000000fe696805c55dda18
Content-Type: text/plain; charset="UTF-8"

patch updated with Tom's suggestions - let me know if I have missed anything.


On Tue, Jun 22, 2021 at 10:18 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 6/21/2021 5:32 PM, Steve French wrote:
> >      There were two places where we weren't checking for error
> >      (e.g. ERESTARTSYS) while waiting for rdma resolution.
> >
> >      Addresses-Coverity: 1462165 ("Unchecked return value")
> >     Signed-off-by: Steve French <stfrench@microsoft.com>
> >
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> > index 10dfe5006792..ae07732f750f 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -572,8 +572,11 @@ static struct rdma_cm_id *smbd_create_id(
> >                  log_rdma_event(ERR, "rdma_resolve_addr() failed %i\n", rc);
> >                  goto out;
> >          }
> > -       wait_for_completion_interruptible_timeout(
> > +       rc = wait_for_completion_interruptible_timeout(
> >                  &info->ri_done, msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT));
> > +       /* -ERESTARTSYS, returned when interrupted, is the only rc mentioned */
>
> Suggest the same comment text as the one below, this one seems uncertain.
>
> > +       if (rc < 0)
> > +               goto out;
> >          rc = info->ri_rc;
> >          if (rc) {
> >                  log_rdma_event(ERR, "rdma_resolve_addr() completed %i\n", rc);
> > @@ -586,8 +589,10 @@ static struct rdma_cm_id *smbd_create_id(
> >                  log_rdma_event(ERR, "rdma_resolve_route() failed %i\n", rc);
> >                  goto out;
> >          }
> > -       wait_for_completion_interruptible_timeout(
> > +       rc = wait_for_completion_interruptible_timeout(
> >                  &info->ri_done, msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT));
> > +       if (rc < 0)  /* e.g. if interrupted and returns -ERESTARTSYS */
>
> delete "and"?
>
> > +               goto out
>
> Missing a semicolon.     ^^^
>
> >          rc = info->ri_rc;
> >          if (rc) {
> >                  log_rdma_event(ERR, "rdma_resolve_route() completed %i\n", rc);
> >
> >
>
> One meta-comment. There's no message logged for these ERESTARTSYS cases.
> That might be confusing in the log, if they lead to failure.
>
> Reviewed-By: Tom Talpey <tom@talpey.com>
>
> Tom.


-- 
Thanks,

Steve

--000000000000fe696805c55dda18
Content-Type: text/x-patch; charset="US-ASCII"; 
	name="0001-smbdirect-missing-rc-checks-while-waiting-for-rdma-e.patch"
Content-Disposition: attachment; 
	filename="0001-smbdirect-missing-rc-checks-while-waiting-for-rdma-e.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_kq8ayoso0>
X-Attachment-Id: f_kq8ayoso0

RnJvbSAwNTU1YjIyMTUyOGU5Y2IxMWY1NzY2ZGNkZWUxOWM4MDkxODdlNDJlIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBTdGV2ZSBGcmVuY2ggPHN0ZnJlbmNoQG1pY3Jvc29mdC5jb20+
CkRhdGU6IE1vbiwgMjEgSnVuIDIwMjEgMTY6MjU6MjAgLTA1MDAKU3ViamVjdDogW1BBVENIXSBz
bWJkaXJlY3Q6IG1pc3NpbmcgcmMgY2hlY2tzIHdoaWxlIHdhaXRpbmcgZm9yIHJkbWEgZXZlbnRz
CgpUaGVyZSB3ZXJlIHR3byBwbGFjZXMgd2hlcmUgd2Ugd2VyZW4ndCBjaGVja2luZyBmb3IgZXJy
b3IKKGUuZy4gRVJFU1RBUlRTWVMpIHdoaWxlIHdhaXRpbmcgZm9yIHJkbWEgcmVzb2x1dGlvbi4K
CkFkZHJlc3Nlcy1Db3Zlcml0eTogMTQ2MjE2NSAoIlVuY2hlY2tlZCByZXR1cm4gdmFsdWUiKQpS
ZXZpZXdlZC1ieTogVG9tIFRhbHBleSA8dG9tQHRhbHBleS5jb20+ClJldmlld2VkLWJ5OiBMb25n
IExpIDxsb25nbGlAbWljcm9zb2Z0LmNvbT4KU2lnbmVkLW9mZi1ieTogU3RldmUgRnJlbmNoIDxz
dGZyZW5jaEBtaWNyb3NvZnQuY29tPgotLS0KIGZzL2NpZnMvc21iZGlyZWN0LmMgfCAxNCArKysr
KysrKysrKystLQogMSBmaWxlIGNoYW5nZWQsIDEyIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25z
KC0pCgpkaWZmIC0tZ2l0IGEvZnMvY2lmcy9zbWJkaXJlY3QuYyBiL2ZzL2NpZnMvc21iZGlyZWN0
LmMKaW5kZXggMTBkZmU1MDA2NzkyLi4zMWVmNjRlYjdmYmIgMTAwNjQ0Ci0tLSBhL2ZzL2NpZnMv
c21iZGlyZWN0LmMKKysrIGIvZnMvY2lmcy9zbWJkaXJlY3QuYwpAQCAtNTcyLDggKzU3MiwxMyBA
QCBzdGF0aWMgc3RydWN0IHJkbWFfY21faWQgKnNtYmRfY3JlYXRlX2lkKAogCQlsb2dfcmRtYV9l
dmVudChFUlIsICJyZG1hX3Jlc29sdmVfYWRkcigpIGZhaWxlZCAlaVxuIiwgcmMpOwogCQlnb3Rv
IG91dDsKIAl9Ci0Jd2FpdF9mb3JfY29tcGxldGlvbl9pbnRlcnJ1cHRpYmxlX3RpbWVvdXQoCisJ
cmMgPSB3YWl0X2Zvcl9jb21wbGV0aW9uX2ludGVycnVwdGlibGVfdGltZW91dCgKIAkJJmluZm8t
PnJpX2RvbmUsIG1zZWNzX3RvX2ppZmZpZXMoUkRNQV9SRVNPTFZFX1RJTUVPVVQpKTsKKwkvKiBl
LmcuIGlmIGludGVycnVwdGVkIHJldHVybnMgLUVSRVNUQVJUU1lTICovCisJaWYgKHJjIDwgMCkg
eworCQlsb2dfcmRtYV9ldmVudChFUlIsICJyZG1hX3Jlc29sdmVfYWRkciB0aW1lb3V0IHJjOiAl
aVxuIiwgcmMpOworCQlnb3RvIG91dDsKKwl9CiAJcmMgPSBpbmZvLT5yaV9yYzsKIAlpZiAocmMp
IHsKIAkJbG9nX3JkbWFfZXZlbnQoRVJSLCAicmRtYV9yZXNvbHZlX2FkZHIoKSBjb21wbGV0ZWQg
JWlcbiIsIHJjKTsKQEAgLTU4Niw4ICs1OTEsMTMgQEAgc3RhdGljIHN0cnVjdCByZG1hX2NtX2lk
ICpzbWJkX2NyZWF0ZV9pZCgKIAkJbG9nX3JkbWFfZXZlbnQoRVJSLCAicmRtYV9yZXNvbHZlX3Jv
dXRlKCkgZmFpbGVkICVpXG4iLCByYyk7CiAJCWdvdG8gb3V0OwogCX0KLQl3YWl0X2Zvcl9jb21w
bGV0aW9uX2ludGVycnVwdGlibGVfdGltZW91dCgKKwlyYyA9IHdhaXRfZm9yX2NvbXBsZXRpb25f
aW50ZXJydXB0aWJsZV90aW1lb3V0KAogCQkmaW5mby0+cmlfZG9uZSwgbXNlY3NfdG9famlmZmll
cyhSRE1BX1JFU09MVkVfVElNRU9VVCkpOworCS8qIGUuZy4gaWYgaW50ZXJydXB0ZWQgcmV0dXJu
cyAtRVJFU1RBUlRTWVMgKi8KKwlpZiAocmMgPCAwKSAgeworCQlsb2dfcmRtYV9ldmVudChFUlIs
ICJyZG1hX3Jlc29sdmVfYWRkciB0aW1lb3V0IHJjOiAlaVxuIiwgcmMpOworCQlnb3RvIG91dDsK
Kwl9CiAJcmMgPSBpbmZvLT5yaV9yYzsKIAlpZiAocmMpIHsKIAkJbG9nX3JkbWFfZXZlbnQoRVJS
LCAicmRtYV9yZXNvbHZlX3JvdXRlKCkgY29tcGxldGVkICVpXG4iLCByYyk7Ci0tIAoyLjMwLjIK
Cg==
--000000000000fe696805c55dda18--