netdev - Re: [PATCH v3] fix locking regression in ipx_sendmsg and ipx

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]

Date:	Wed, 19 Nov 2014 11:50:46 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Jiri Bohac <jbohac@...e.cz>
Cc:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH v3] fix locking regression in ipx_sendmsg and ipx_recvmsg

On Wednesday 19 November 2014 11:38:14 Jiri Bohac wrote:
> This fixes an old regression introduced by commit
> b0d0d915 (ipx: remove the BKL).
> 
> When a recvmsg syscall blocks waiting for new data, no data can be sent on the
> same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
> 
> This breaks mars-nwe (NetWare emulator):
> - the ncpserv process reads the request using recvmsg
> - ncpserv forks and spawns nwconn
> - ncpserv calls a (blocking) recvmsg and waits for new requests
> - nwconn deadlocks in sendmsg on the same socket 
> 
> Commit b0d0d915 has simply replaced BKL locking with
> lock_sock/release_sock. Unlike now, BKL got unlocked while
> sleeping, so a blocking recvmsg did not block a concurrent
> sendmsg.
> 
> Only keep the socket locked while actually working with the socket data and
> release it prior to calling skb_recv_datagram(). 
> 
> Signed-off-by: Jiri Bohac <jbohac@...e.cz>

Looks correct to me and simple enough,

Reviewed-by: Arnd Bergmann <arnd@...db.de>

> diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
> index a0c7536..d0725d9 100644
> --- a/net/ipx/af_ipx.c
> +++ b/net/ipx/af_ipx.c
> @@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	struct ipxhdr *ipx = NULL;
>  	struct sk_buff *skb;
>  	int copied, rc;
> +	int locked = 1;
>  
>  	lock_sock(sk);
>  	/* put the autobinding in */
> @@ -1790,6 +1791,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	if (sock_flag(sk, SOCK_ZAPPED))
>  		goto out;
>  
> +	release_sock(sk);
> +	locked = 0;
>  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
>  				flags & MSG_DONTWAIT, &rc);
>  	if (!skb) {
> @@ -1825,7 +1828,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
>  out_free:
>  	skb_free_datagram(sk, skb);
>  out:
> -	release_sock(sk);
> +	if (locked)
> +		release_sock(sk);
>  	return rc;
>  }

I don't like the idea of having a local flag for this, and would still
prefer the simpler version of taking the lock again even if it's not
needed, but your version is probably good enough unless Dave wants
you to do a v4 for this.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html