Re: [PATCH] cifs: Implement is_network_name_deleted for SMB1

["Pali Rohár" <pali@kernel.org>]

On Sunday 22 December 2024 18:14:31 Pali Rohár wrote:
> This change allows Linux SMB1 client to autoreconnect the share when it is
> modified on server by admin operation which removes and re-adds it.
> 
> Implementation is reused from SMB2+ is_network_name_deleted callback. There
> are just adjusted checks for error codes and access to struct smb_hdr.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/smb1ops.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
> index 0533aca770fc..76e7353f2a72 100644
> --- a/fs/smb/client/smb1ops.c
> +++ b/fs/smb/client/smb1ops.c
> @@ -14,6 +14,8 @@
>  #include "cifspdu.h"
>  #include "cifs_unicode.h"
>  #include "fs_context.h"
> +#include "nterr.h"
> +#include "smberr.h"
>  
>  /*
>   * An NT cancel request header looks just like the original request except:
> @@ -1075,6 +1077,47 @@ cifs_make_node(unsigned int xid, struct inode *inode,
>  				  full_path, mode, dev);
>  }
>  
> +static bool
> +cifs_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> +{
> +	struct smb_hdr *shdr = (struct smb_hdr *)buf;
> +	struct TCP_Server_Info *pserver;
> +	struct cifs_ses *ses;
> +	struct cifs_tcon *tcon;
> +
> +	if (shdr->Flags2 & SMBFLG2_ERR_STATUS) {
> +		if (shdr->Status.CifsError != cpu_to_le32(NT_STATUS_NETWORK_NAME_DELETED))
> +			return false;
> +	} else {
> +		if (shdr->Status.DosError.ErrorClass != ERRSRV ||
> +		    shdr->Status.DosError.Error != cpu_to_le16(ERRinvtid))
> +			return false;
> +	}
> +
> +	/* If server is a channel, select the primary channel */
> +	pserver = SERVER_IS_CHAN(server) ? server->primary_server : server;
> +
> +	spin_lock(&cifs_tcp_ses_lock);
> +	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
> +		if (cifs_ses_exiting(ses))
> +			continue;
> +		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
> +			if (tcon->tid == le32_to_cpu(shdr->Tid)) {

Kernel test robot complained on this line about endianity issues. I have
looked why, and I figured out that for smb1 the tcon->tid value is
stored in the little endian, but for smb2+ it is stored in cpu endian.
Hence this line should be:

-			if (tcon->tid == le32_to_cpu(shdr->Tid)) {
+			if (tcon->tid == shdr->Tid) {

I will include this fixup into v2 when needed (during next rebase).

Note that it is hard to catch such endianity issues as majority of user
machines are little endian, so such issues are hidden.

> +				spin_lock(&tcon->tc_lock);
> +				tcon->need_reconnect = true;
> +				spin_unlock(&tcon->tc_lock);
> +				spin_unlock(&cifs_tcp_ses_lock);
> +				pr_warn_once("Server share %s deleted.\n",
> +					     tcon->tree_name);
> +				return true;
> +			}
> +		}
> +	}
> +	spin_unlock(&cifs_tcp_ses_lock);
> +
> +	return false;
> +}
> +
>  struct smb_version_operations smb1_operations = {
>  	.send_cancel = send_nt_cancel,
>  	.compare_fids = cifs_compare_fids,
> @@ -1159,6 +1202,7 @@ struct smb_version_operations smb1_operations = {
>  	.get_acl_by_fid = get_cifs_acl_by_fid,
>  	.set_acl = set_cifs_acl,
>  	.make_node = cifs_make_node,
> +	.is_network_name_deleted = cifs_is_network_name_deleted,
>  };
>  
>  struct smb_version_values smb1_values = {
> -- 
> 2.20.1
>