From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7ACBCC43381 for ; Tue, 26 Feb 2019 06:38:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 53C832147C for ; Tue, 26 Feb 2019 06:38:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726188AbfBZGiI (ORCPT ); Tue, 26 Feb 2019 01:38:08 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:57376 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725881AbfBZGiI (ORCPT ); Tue, 26 Feb 2019 01:38:08 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.92 #3 (Red Hat Linux)) id 1gyWNQ-0005nw-9b; Tue, 26 Feb 2019 06:38:04 +0000 Date: Tue, 26 Feb 2019 06:38:04 +0000 From: Al Viro To: Rainer Weikusat , Jason Baron Cc: netdev@vger.kernel.org Subject: Re: [RFC] nasty corner case in unix_dgram_sendmsg() Message-ID: <20190226063804.GI2217@ZenIV.linux.org.uk> References: <20190225035121.GH2217@ZenIV.linux.org.uk> <20190226062817.GA17962@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190226062817.GA17962@ZenIV.linux.org.uk> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote: > 2) the logics in sendmsg is very odd: > * if we'd detected n:1 send *and* found that we need to > wait, do so (not using the peer_wake - other's peer_wait is not > going away). No questions there. > * if we'd detected n:1 send *and* found that we need to > wait, but don't have any remaining timeout, we lock both ends > and check if unix_peer(sk) is (now) not equal to other, either > because it never had or because we'd been hit by connect(2) while > we'd been doing the locking. In that case we fail with -EAGAIN. > Fair enough, but > * if after relocking we see that unix_peer(sk) now > is equal to other, we arrange for wakeup forwarding from other's > peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN. > Huh? What's the point? The only thing that depends upon that > wakeup forwarding is poll, and _that_ will set the forwarding up > on its own. > * if we'd failed (either because other is dead now or > because its queue is not full), we go back to restart_locked. > If it's dead, we'll sod off with ECONNREFUSED, if it's not > full anymore, we'll send the damn thing. > > All of that comes at the cost of considerable headache in > unix_dgram_sendmsg() - flag-conditional locking, etc. Why do > we bother? What's wrong with simple "n:1 case detected, queue > full, no timeout left, return -EAGAIN and be done with that"? > > IDGI... Am I missing something subtle going on here? > > I understand what peer_wake is for, and the poll side of things > is fine; it's sendmsg one that looks weird. What's going on > there? What I have in mind is something like this (for that part of the issues): diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 74d1eed7cbd4..85d72ea79924 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1635,7 +1635,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, long timeo; struct scm_cookie scm; int data_len = 0; - int sk_locked; wait_for_unix_gc(); err = scm_send(sock, msg, &scm, false); @@ -1713,9 +1712,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, goto out_free; } - sk_locked = 0; unix_state_lock(other); -restart_locked: + err = -EPERM; if (!unix_may_send(sk, other)) goto out_unlock; @@ -1728,8 +1726,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, unix_state_unlock(other); sock_put(other); - if (!sk_locked) - unix_state_lock(sk); + unix_state_lock(sk); err = 0; if (unix_peer(sk) == other) { @@ -1767,36 +1764,19 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, */ if (other != sk && unlikely(unix_peer(other) != sk && unix_recvq_full(other))) { - if (timeo) { - timeo = unix_wait_for_peer(other, timeo); - - err = sock_intr_errno(timeo); - if (signal_pending(current)) - goto out_free; - - goto restart; - } - - if (!sk_locked) { - unix_state_unlock(other); - unix_state_double_lock(sk, other); - } - - if (unix_peer(sk) != other || - unix_dgram_peer_wake_me(sk, other)) { + if (!timeo) { err = -EAGAIN; - sk_locked = 1; goto out_unlock; } - if (!sk_locked) { - sk_locked = 1; - goto restart_locked; - } - } + timeo = unix_wait_for_peer(other, timeo); - if (unlikely(sk_locked)) - unix_state_unlock(sk); + err = sock_intr_errno(timeo); + if (signal_pending(current)) + goto out_free; + + goto restart; + } if (sock_flag(other, SOCK_RCVTSTAMP)) __net_timestamp(skb); @@ -1809,8 +1789,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, return len; out_unlock: - if (sk_locked) - unix_state_unlock(sk); unix_state_unlock(other); out_free: kfree_skb(skb);