netdev - Re: [PATCH net] net/smc: Transfer remaining wait queue entries during fallback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YY5z4H5/CVpRtrwh@osiris>
Date:   Fri, 12 Nov 2021 15:02:08 +0100
From:   Heiko Carstens <hca@...ux.ibm.com>
To:     Wen Gu <guwen@...ux.alibaba.com>
Cc:     kgraul@...ux.ibm.com, davem@...emloft.net, kuba@...nel.org,
        linux-s390@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, tonylu@...ux.alibaba.com,
        dust.li@...ux.alibaba.com, xuanzhuo@...ux.alibaba.com
Subject: Re: [PATCH net] net/smc: Transfer remaining wait queue entries
 during fallback

On Fri, Nov 12, 2021 at 11:30:39AM +0800, Wen Gu wrote:
...
> +	wait_queue_head_t *smc_wait = sk_sleep(&smc->sk);
> +	wait_queue_head_t *clc_wait = sk_sleep(smc->clcsock->sk);
> +	unsigned long flags;
> +
>  	smc->use_fallback = true;
>  	smc->fallback_rsn = reason_code;
>  	smc_stat_fallback(smc);
> @@ -571,6 +575,16 @@ static void smc_switch_to_fallback(struct smc_sock *smc, int reason_code)
>  		smc->clcsock->file->private_data = smc->clcsock;
>  		smc->clcsock->wq.fasync_list =
>  			smc->sk.sk_socket->wq.fasync_list;
> +
> +		/* There might be some wait queue entries remaining
> +		 * in smc socket->wq, which should be removed to
> +		 * clcsocket->wq during the fallback.
> +		 */
> +		spin_lock_irqsave(&smc_wait->lock, flags);
> +		spin_lock_irqsave(&clc_wait->lock, flags);
> +		list_splice_init(&smc_wait->head, &clc_wait->head);
> +		spin_unlock_irqrestore(&clc_wait->lock, flags);
> +		spin_unlock_irqrestore(&smc_wait->lock, flags);

No idea if the rest of the patch makes sense, however this usage of
spin_lock_irqsave() is not correct. The second spin_lock_irqsave()
would always save a state with irqs disabled into "flags", and
therefore this path would always be left with irqs disabled,
regardless if irqs were enabled or disabled when entering.

You need to change it to something like

> +		spin_lock_irqsave(&smc_wait->lock, flags);
> +		spin_lock(&clc_wait->lock);
> +		list_splice_init(&smc_wait->head, &clc_wait->head);
> +		spin_unlock(&clc_wait->lock);
> +		spin_unlock_irqrestore(&smc_wait->lock, flags);