[0/1] cifs: fix credits leak for SMB1 oplock breaks

Message

Ronnie Sahlberg April 26, 2019, 12:32 a.m. UTC

Pavel, List

Here is an update after the email discussions.
This is minimal patch that adds a new flag to specifically indicate
that there will not be a response to this PDU and thus we can immediately
add the credits back after sending in compound_send_recv()
This flag is only set for SMB1 oplock breaks so this will not affect anything
else.

Additionally, we no longer need to check the CIFS_ASYNC_OP flag in
compound_send_recv() so we can remove that conditional.

This is the smallest patch which fixes the actual smb1 oplock bug thus we should
be able to get it into an rc, and stable.
But we need more but that should wait until the next merge window.


The next steps we should do is:
Remove CIFSSMBNotify() completely and with that also the check for
CIFS_ASYNC_OP in SendReceive()

When that is done, the only thing that CIFS_ASYNC_OP does is make sure
we do not block in wait_for_credits().
Which means we could rename this flag to CIFS_NON_BLOCKING.

We should also rename CIFS_NO_RSP to CIFS_NO_RSP_BUF to make it more clear that
there will be a response status code but there won't be any or we don't care about any response buffer.


Thus the semantics will become:
* cifs_call_async() : if you want async handling of responses
* CIFS_NO_SRV_RSP: There won't be a response so do not wait for a reply. (only used by SMB1 oplocks)
* CIFS_ASYNC_OP/CIFS_NON_BLOCKING: do not block waiting for available credits.

Comments

Pavel Shilovsky April 26, 2019, 4:35 p.m. UTC | #1

чт, 25 апр. 2019 г. в 18:42, Ronnie Sahlberg <[e-mail]>:
>
> Pavel, List
>
> Here is an update after the email discussions.
> This is minimal patch that adds a new flag to specifically indicate
> that there will not be a response to this PDU and thus we can immediately
> add the credits back after sending in compound_send_recv()
> This flag is only set for SMB1 oplock breaks so this will not affect anything
> else.
>
> Additionally, we no longer need to check the CIFS_ASYNC_OP flag in
> compound_send_recv() so we can remove that conditional.
>
> This is the smallest patch which fixes the actual smb1 oplock bug thus we should
> be able to get it into an rc, and stable.
> But we need more but that should wait until the next merge window.
>
>
> The next steps we should do is:
> Remove CIFSSMBNotify() completely and with that also the check for
> CIFS_ASYNC_OP in SendReceive()
>
> When that is done, the only thing that CIFS_ASYNC_OP does is make sure
> we do not block in wait_for_credits().
> Which means we could rename this flag to CIFS_NON_BLOCKING.
>
> We should also rename CIFS_NO_RSP to CIFS_NO_RSP_BUF to make it more clear that
> there will be a response status code but there won't be any or we don't care about any response buffer.
>
>
> Thus the semantics will become:
> * cifs_call_async() : if you want async handling of responses
> * CIFS_NO_SRV_RSP: There won't be a response so do not wait for a reply. (only used by SMB1 oplocks)
> * CIFS_ASYNC_OP/CIFS_NON_BLOCKING: do not block waiting for available credits.
>

Agree, sounds like a good plan. Thanks for summarizing!

--
Best regards,
Pavel Shilovsky