| Message ID | a9890f37f327c3765a58247636e6586c7439ae62.1510736246.git.lucien.xin@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [PATCHv2,net] sctp: use the right sk after waking up from wait_buf sleep | expand |
On Wed, Nov 15, 2017 at 04:57:26PM +0800, Xin Long wrote: > Commit dfcb9f4f99f1 ("sctp: deny peeloff operation on asocs with threads > sleeping on it") fixed the race between peeloff and wait sndbuf by > checking waitqueue_active(&asoc->wait) in sctp_do_peeloff(). > > But it actually doesn't work, as even if waitqueue_active returns false > the waiting sndbuf thread may still not yet hold sk lock. After asoc is > peeled off, sk is not asoc->base.sk any more, then to hold the old sk > lock couldn't make assoc safe to access. > > This patch is to fix this by changing to hold the new sk lock if sk is > not asoc->base.sk, meanwhile, also set the sk in sctp_sendmsg with the > new sk. > > With this fix, there is no more race between peeloff and waitbuf, the > check 'waitqueue_active' in sctp_do_peeloff can be removed. > > Thanks Marcelo and Neil for making this clear. > > v1->v2: > fix it by changing to lock the new sock instead of adding a flag in asoc. > > Suggested-by: Neil Horman <nhorman@tuxdriver.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> Acked-by: Neil Horman <nhorman@tuxdriver.com>
From: Xin Long <lucien.xin@gmail.com> Date: Wed, 15 Nov 2017 16:57:26 +0800 > Commit dfcb9f4f99f1 ("sctp: deny peeloff operation on asocs with threads > sleeping on it") fixed the race between peeloff and wait sndbuf by > checking waitqueue_active(&asoc->wait) in sctp_do_peeloff(). > > But it actually doesn't work, as even if waitqueue_active returns false > the waiting sndbuf thread may still not yet hold sk lock. After asoc is > peeled off, sk is not asoc->base.sk any more, then to hold the old sk > lock couldn't make assoc safe to access. > > This patch is to fix this by changing to hold the new sk lock if sk is > not asoc->base.sk, meanwhile, also set the sk in sctp_sendmsg with the > new sk. > > With this fix, there is no more race between peeloff and waitbuf, the > check 'waitqueue_active' in sctp_do_peeloff can be removed. > > Thanks Marcelo and Neil for making this clear. > > v1->v2: > fix it by changing to lock the new sock instead of adding a flag in asoc. > > Suggested-by: Neil Horman <nhorman@tuxdriver.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> Applied.
diff --git a/net/sctp/socket.c b/net/sctp/socket.c index e62251f..14c28fb 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -83,8 +83,8 @@ /* Forward declarations for internal helper functions. */ static int sctp_writeable(struct sock *sk); static void sctp_wfree(struct sk_buff *skb); -static int sctp_wait_for_sndbuf(struct sctp_association *, long *timeo_p, - size_t msg_len); +static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, + size_t msg_len, struct sock **orig_sk); static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p); static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p); static int sctp_wait_for_accept(struct sock *sk, long timeo); @@ -1962,7 +1962,8 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len) timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT); if (!sctp_wspace(asoc)) { - err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len); + /* sk can be changed by peel off when waiting for buf. */ + err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len, &sk); if (err) { if (err == -ESRCH) { /* asoc is already dead. */ @@ -4949,12 +4950,6 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) if (!asoc) return -EINVAL; - /* If there is a thread waiting on more sndbuf space for - * sending on this asoc, it cannot be peeled. - */ - if (waitqueue_active(&asoc->wait)) - return -EBUSY; - /* An association cannot be branched off from an already peeled-off * socket, nor is this supported for tcp style sockets. */ @@ -7828,7 +7823,7 @@ void sctp_sock_rfree(struct sk_buff *skb) /* Helper function to wait for space in the sndbuf. */ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, - size_t msg_len) + size_t msg_len, struct sock **orig_sk) { struct sock *sk = asoc->base.sk; int err = 0; @@ -7862,11 +7857,17 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, release_sock(sk); current_timeo = schedule_timeout(current_timeo); lock_sock(sk); + if (sk != asoc->base.sk) { + release_sock(sk); + sk = asoc->base.sk; + lock_sock(sk); + } *timeo_p = current_timeo; } out: + *orig_sk = sk; finish_wait(&asoc->wait, &wait); /* Release the association's refcnt. */
Commit dfcb9f4f99f1 ("sctp: deny peeloff operation on asocs with threads sleeping on it") fixed the race between peeloff and wait sndbuf by checking waitqueue_active(&asoc->wait) in sctp_do_peeloff(). But it actually doesn't work, as even if waitqueue_active returns false the waiting sndbuf thread may still not yet hold sk lock. After asoc is peeled off, sk is not asoc->base.sk any more, then to hold the old sk lock couldn't make assoc safe to access. This patch is to fix this by changing to hold the new sk lock if sk is not asoc->base.sk, meanwhile, also set the sk in sctp_sendmsg with the new sk. With this fix, there is no more race between peeloff and waitbuf, the check 'waitqueue_active' in sctp_do_peeloff can be removed. Thanks Marcelo and Neil for making this clear. v1->v2: fix it by changing to lock the new sock instead of adding a flag in asoc. Suggested-by: Neil Horman <nhorman@tuxdriver.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/socket.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)