LGTM
reviewed-by me.
Good catch about the race!
On Mon, 19 Sept 2022 at 09:54, Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
>
> There's a race when cifs_readv_receive() might dequeue the mid,
> and mid->callback(), called from demultiplex thread, will try to
> access it to verify the signature before the mid is actually
> released/deleted.
>
> Currently the signature verification fails, but the verification
> shouldn't have happened at all because the mid was deleted because
> of an error, and hence not really supposed to be passed to
> ->callback(). There are no further errors because the mid is
> effectivelly gone by the end of the callback.
>
> This patch checks if the mid doesn't have the MID_DELETED flag set (by
> dequeue_mid()) right before trying to verify the signature. According to
> my tests, trying to check it earlier, e.g. after the ->receive() call in
> cifs_demultiplex_thread, will fail most of the time as dequeue_mid()
> might not have been called yet.
>
> This behaviour can be seen in xfstests generic/465, for example, where
> mids with STATUS_END_OF_FILE (-ENODATA) are dequeued and supposed to be
> discarded, but instead have their signature computed, but mismatched.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
> ---
> fs/cifs/cifssmb.c | 2 +-
> fs/cifs/smb2pdu.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index addf3fc62aef..116f6afe33c6 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1308,7 +1308,7 @@ cifs_readv_callback(struct mid_q_entry *mid)
> switch (mid->mid_state) {
> case MID_RESPONSE_RECEIVED:
> /* result already set, check signature */
> - if (server->sign) {
> + if (server->sign && !(mid->mid_flags & MID_DELETED)) {
> int rc = 0;
>
> rc = cifs_verify_signature(&rqst, server,
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 5da0b596c8a0..394996c4f729 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -4136,7 +4136,7 @@ smb2_readv_callback(struct mid_q_entry *mid)
> credits.value = le16_to_cpu(shdr->CreditRequest);
> credits.instance = server->reconnect_instance;
> /* result already set, check signature */
> - if (server->sign && !mid->decrypted) {
> + if (server->sign && !mid->decrypted && !(mid->mid_flags & MID_DELETED)) {
> int rc;
>
> rc = smb2_verify_signature(&rqst, server);
> --
> 2.35.3
>
