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);
