| Message ID | 20201013171849.236025-2-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: dccp: fix structure use-after-free | expand |
On 13/10/2020 19:18, Kleber Sacilotto de Souza wrote: > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > del_timer_sync can't be used is because this relies on keeping a reference > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > during disconnect, the timer should really belong to struct dccp_sock. > > This addresses CVE-2020-16119. > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-bd: Richard Sailer <richard_siegfried@systemli.org> Implementation and concept looks fine to me
On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > del_timer_sync can't be used is because this relies on keeping a reference > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > during disconnect, the timer should really belong to struct dccp_sock. > > This addresses CVE-2020-16119. > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) Presumably you chose this commit because the fix won't apply beyond it? But it really fixes 2677d2067731 (dccp: don't free.. right? > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > del_timer_sync can't be used is because this relies on keeping a reference > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > during disconnect, the timer should really belong to struct dccp_sock. > > This addresses CVE-2020-16119. > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> I've been mulling over this fix. The layering violation really doesn't sit well. We're reusing the timer object. What if we are really unlucky, the fires and gets blocked by a cosmic ray just as it's about to try to lock the socket, then user manages to reconnect, and timer starts again. Potentially with a different CCID algo altogether? Is disconnect ever called under the BH lock? Maybe plumb a bool argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() when called from disconnect()? Or do refcounting on ccid_priv so that the timer holds both the socket and the priv?
On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote: > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > > del_timer_sync can't be used is because this relies on keeping a reference > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > > during disconnect, the timer should really belong to struct dccp_sock. > > > > This addresses CVE-2020-16119. > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > I've been mulling over this fix. > > The layering violation really doesn't sit well. > > We're reusing the timer object. What if we are really unlucky, the > fires and gets blocked by a cosmic ray just as it's about to try to > lock the socket, then user manages to reconnect, and timer starts > again. Potentially with a different CCID algo altogether? > > Is disconnect ever called under the BH lock? Maybe plumb a bool > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() > when called from disconnect()? > > Or do refcounting on ccid_priv so that the timer holds both the socket > and the priv? Sorry about too late a response. I was on vacation, then came back and spent a couple of days testing this further, and had to switch to other tasks. So, while testing this, I had to resort to tricks like having a very small expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF. That's with or without the first of the second patch. I also tried to refcount ccid instead of the socket, keeping the timer on the ccid, but that still hit the UAF, and that's when I had to switch tasks. Oh, and in the meantime, I found one or two other fixes that we should apply, will send them shortly. But I would argue that we should apply the revert as it addresses the CVE, without really regressing the other UAF, as I argued. Does that make sense? Thank you. Cascardo.
On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote: > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote: > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > > > del_timer_sync can't be used is because this relies on keeping a reference > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > > > during disconnect, the timer should really belong to struct dccp_sock. > > > > > > This addresses CVE-2020-16119. > > > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > > > I've been mulling over this fix. > > > > The layering violation really doesn't sit well. > > > > We're reusing the timer object. What if we are really unlucky, the > > fires and gets blocked by a cosmic ray just as it's about to try to > > lock the socket, then user manages to reconnect, and timer starts > > again. Potentially with a different CCID algo altogether? > > > > Is disconnect ever called under the BH lock? Maybe plumb a bool > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() > > when called from disconnect()? > > > > Or do refcounting on ccid_priv so that the timer holds both the socket > > and the priv? > > Sorry about too late a response. I was on vacation, then came back and spent a > couple of days testing this further, and had to switch to other tasks. > > So, while testing this, I had to resort to tricks like having a very small > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF. > That's with or without the first of the second patch. > > I also tried to refcount ccid instead of the socket, keeping the timer on the > ccid, but that still hit the UAF, and that's when I had to switch tasks. Hm, not instead, as well. I think trying cancel the timer _sync from the disconnect path would be the simplest solution, tho. > Oh, and in the meantime, I found one or two other fixes that we > should apply, will send them shortly. > > But I would argue that we should apply the revert as it addresses the > CVE, without really regressing the other UAF, as I argued. Does that > make sense? We can - it's always a little strange to go from one bug to a different without a fix - but the justification being that while the previous UAF required a race condition the new one is actually worst because it can be triggered reliably?
On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote: > On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote: > > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote: > > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > > > > del_timer_sync can't be used is because this relies on keeping a reference > > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > > > > during disconnect, the timer should really belong to struct dccp_sock. > > > > > > > > This addresses CVE-2020-16119. > > > > > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > > > > > I've been mulling over this fix. > > > > > > The layering violation really doesn't sit well. > > > > > > We're reusing the timer object. What if we are really unlucky, the > > > fires and gets blocked by a cosmic ray just as it's about to try to > > > lock the socket, then user manages to reconnect, and timer starts > > > again. Potentially with a different CCID algo altogether? > > > > > > Is disconnect ever called under the BH lock? Maybe plumb a bool > > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() > > > when called from disconnect()? > > > > > > Or do refcounting on ccid_priv so that the timer holds both the socket > > > and the priv? > > > > Sorry about too late a response. I was on vacation, then came back and spent a > > couple of days testing this further, and had to switch to other tasks. > > > > So, while testing this, I had to resort to tricks like having a very small > > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF. > > That's with or without the first of the second patch. > > > > I also tried to refcount ccid instead of the socket, keeping the timer on the > > ccid, but that still hit the UAF, and that's when I had to switch tasks. > > Hm, not instead, as well. I think trying cancel the timer _sync from > the disconnect path would be the simplest solution, tho. > I don't think so. On other paths, we would still have the possibility that: CPU1: timer expires and is about to run CPU2: calls stop_timer (which does not stop anything) and frees ccid CPU1: timer runs and uses freed ccid And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would not be able to call stop_timer_sync. > > Oh, and in the meantime, I found one or two other fixes that we > > should apply, will send them shortly. > > > > But I would argue that we should apply the revert as it addresses the > > CVE, without really regressing the other UAF, as I argued. Does that > > make sense? > > We can - it's always a little strange to go from one bug to a different > without a fix - but the justification being that while the previous UAF > required a race condition the new one is actually worst because it can > be triggered reliably? Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1 ("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might happen, because the timer might trigger right after we free the ccid struct. And, yes, on the other hand, we can reliably launch the DoS attack that is fixed by the revert of that commit. Thanks. Cascardo.
On Mon, 9 Nov 2020 18:09:09 -0300 Thadeu Lima de Souza Cascardo wrote: > On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote: > > On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote: > > > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote: > > > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > > > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > > > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > > > > > del_timer_sync can't be used is because this relies on keeping a reference > > > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > > > > > during disconnect, the timer should really belong to struct dccp_sock. > > > > > > > > > > This addresses CVE-2020-16119. > > > > > > > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > > > > > > > I've been mulling over this fix. > > > > > > > > The layering violation really doesn't sit well. > > > > > > > > We're reusing the timer object. What if we are really unlucky, the > > > > fires and gets blocked by a cosmic ray just as it's about to try to > > > > lock the socket, then user manages to reconnect, and timer starts > > > > again. Potentially with a different CCID algo altogether? > > > > > > > > Is disconnect ever called under the BH lock? Maybe plumb a bool > > > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() > > > > when called from disconnect()? > > > > > > > > Or do refcounting on ccid_priv so that the timer holds both the socket > > > > and the priv? > > > > > > Sorry about too late a response. I was on vacation, then came back and spent a > > > couple of days testing this further, and had to switch to other tasks. > > > > > > So, while testing this, I had to resort to tricks like having a very small > > > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF. > > > That's with or without the first of the second patch. > > > > > > I also tried to refcount ccid instead of the socket, keeping the timer on the > > > ccid, but that still hit the UAF, and that's when I had to switch tasks. > > > > Hm, not instead, as well. I think trying cancel the timer _sync from > > the disconnect path would be the simplest solution, tho. > > I don't think so. On other paths, we would still have the possibility that: > > CPU1: timer expires and is about to run > CPU2: calls stop_timer (which does not stop anything) and frees ccid > CPU1: timer runs and uses freed ccid > > And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would > not be able to call stop_timer_sync. Which paths are those (my memory of this code is waning)? I thought disconnect is only called from the user space side (shutdown syscall). The only other way to terminate the connection is to close the socket, which Eric already fixed by postponing the destruction of ccid in that case. > > > Oh, and in the meantime, I found one or two other fixes that we > > > should apply, will send them shortly. > > > > > > But I would argue that we should apply the revert as it addresses the > > > CVE, without really regressing the other UAF, as I argued. Does that > > > make sense? > > > > We can - it's always a little strange to go from one bug to a different > > without a fix - but the justification being that while the previous UAF > > required a race condition the new one is actually worst because it can > > be triggered reliably? > > Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1 > ("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't > really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might > happen, because the timer might trigger right after we free the ccid struct. > > And, yes, on the other hand, we can reliably launch the DoS attack that is > fixed by the revert of that commit. OK.
On Mon, Nov 09, 2020 at 01:15:54PM -0800, Jakub Kicinski wrote: > On Mon, 9 Nov 2020 18:09:09 -0300 Thadeu Lima de Souza Cascardo wrote: > > On Mon, Nov 09, 2020 at 09:49:38AM -0800, Jakub Kicinski wrote: > > > On Mon, 9 Nov 2020 08:48:28 -0300 Thadeu Lima de Souza Cascardo wrote: > > > > On Fri, Oct 16, 2020 at 03:30:16PM -0700, Jakub Kicinski wrote: > > > > > On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote: > > > > > > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > > > > > > > > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason > > > > > > del_timer_sync can't be used is because this relies on keeping a reference > > > > > > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that > > > > > > during disconnect, the timer should really belong to struct dccp_sock. > > > > > > > > > > > > This addresses CVE-2020-16119. > > > > > > > > > > > > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup()) > > > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > > > > > > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > > > > > > > > > I've been mulling over this fix. > > > > > > > > > > The layering violation really doesn't sit well. > > > > > > > > > > We're reusing the timer object. What if we are really unlucky, the > > > > > fires and gets blocked by a cosmic ray just as it's about to try to > > > > > lock the socket, then user manages to reconnect, and timer starts > > > > > again. Potentially with a different CCID algo altogether? > > > > > > > > > > Is disconnect ever called under the BH lock? Maybe plumb a bool > > > > > argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync() > > > > > when called from disconnect()? > > > > > > > > > > Or do refcounting on ccid_priv so that the timer holds both the socket > > > > > and the priv? > > > > > > > > Sorry about too late a response. I was on vacation, then came back and spent a > > > > couple of days testing this further, and had to switch to other tasks. > > > > > > > > So, while testing this, I had to resort to tricks like having a very small > > > > expire and enqueuing on a different CPU. Then, after some minutes, I hit a UAF. > > > > That's with or without the first of the second patch. > > > > > > > > I also tried to refcount ccid instead of the socket, keeping the timer on the > > > > ccid, but that still hit the UAF, and that's when I had to switch tasks. > > > > > > Hm, not instead, as well. I think trying cancel the timer _sync from > > > the disconnect path would be the simplest solution, tho. > > > > I don't think so. On other paths, we would still have the possibility that: > > > > CPU1: timer expires and is about to run > > CPU2: calls stop_timer (which does not stop anything) and frees ccid > > CPU1: timer runs and uses freed ccid > > > > And those paths, IIUC, may be run under a SoftIRQ on the receive path, so would > > not be able to call stop_timer_sync. > > Which paths are those (my memory of this code is waning)? I thought > disconnect is only called from the user space side (shutdown syscall). > The only other way to terminate the connection is to close the socket, > which Eric already fixed by postponing the destruction of ccid in that > case. dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options -> dccp_feat_parse_options -> dccp_feat_handle_nn_established -> dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid -> ccid_hc_tx_delete > > > > > Oh, and in the meantime, I found one or two other fixes that we > > > > should apply, will send them shortly. > > > > > > > > But I would argue that we should apply the revert as it addresses the > > > > CVE, without really regressing the other UAF, as I argued. Does that > > > > make sense? > > > > > > We can - it's always a little strange to go from one bug to a different > > > without a fix - but the justification being that while the previous UAF > > > required a race condition the new one is actually worst because it can > > > be triggered reliably? > > > > Well, I am arguing here that commit 2677d20677314101293e6da0094ede7b5526d2b1 > > ("dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()") doesn't > > really fix anything. Whenever ccid_hx_tx_delete is called, that UAF might > > happen, because the timer might trigger right after we free the ccid struct. > > > > And, yes, on the other hand, we can reliably launch the DoS attack that is > > fixed by the revert of that commit. > > OK. >
On Mon, 9 Nov 2020 18:31:34 -0300 Thadeu Lima de Souza Cascardo wrote: > > Which paths are those (my memory of this code is waning)? I thought > > disconnect is only called from the user space side (shutdown syscall). > > The only other way to terminate the connection is to close the socket, > > which Eric already fixed by postponing the destruction of ccid in that > > case. > > dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options -> > dccp_feat_parse_options -> dccp_feat_handle_nn_established -> > dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid -> > ccid_hc_tx_delete Well, that's not a disconnect path. There should be no CCID on a disconnected socket, tho, right? Otherwise if we can switch from one active CCID to another then reusing a single timer in struct dccp_sock for both is definitely not safe as I explained in my initial email.
On Mon, Nov 09, 2020 at 02:15:53PM -0800, Jakub Kicinski wrote: > On Mon, 9 Nov 2020 18:31:34 -0300 Thadeu Lima de Souza Cascardo wrote: > > > Which paths are those (my memory of this code is waning)? I thought > > > disconnect is only called from the user space side (shutdown syscall). > > > The only other way to terminate the connection is to close the socket, > > > which Eric already fixed by postponing the destruction of ccid in that > > > case. > > > > dccp_v4_do_rcv -> dccp_rcv_established -> dccp_parse_options -> > > dccp_feat_parse_options -> dccp_feat_handle_nn_established -> > > dccp_feat_activate -> __dccp_feat_activate -> dccp_hdlr_ccid -> > > ccid_hc_tx_delete > > Well, that's not a disconnect path. > > There should be no CCID on a disconnected socket, tho, right? Otherwise > if we can switch from one active CCID to another then reusing a single > timer in struct dccp_sock for both is definitely not safe as I > explained in my initial email. Yeah, I agree with your initial email. The patch I submitted for that fix needs rework, which is what I tried and failed so far. I need to get back to some testing of my latest fix and find out what needs fixing there. But I am also saying that simply doing a del_timer_sync on disconnect paths won't do, because there are non-disconnect paths where there is a CCID that we will remove and replace and that will still trigger a timer UAF. So I have been working on a fix that involves a refcnt on ccid itself. But I want to test that it really fixes the problem and I have spent most of the time finding out a way to trigger the timer in a race with the disconnect path. And that same test has showed me that this timer UAF will happen regardless of commit 2677d20677314101293e6da0094ede7b5526d2b1, which led me into stating that reverting it should be done in any case. I think I can find some time this week to work a little further on the fix for the time UAF. Thanks. Cascardo.
On Tue, 10 Nov 2020 08:19:32 -0300 Thadeu Lima de Souza Cascardo wrote: > Yeah, I agree with your initial email. The patch I submitted for that fix needs > rework, which is what I tried and failed so far. I need to get back to some > testing of my latest fix and find out what needs fixing there. > > But I am also saying that simply doing a del_timer_sync on disconnect paths > won't do, because there are non-disconnect paths where there is a CCID that we > will remove and replace and that will still trigger a timer UAF. > > So I have been working on a fix that involves a refcnt on ccid itself. But I > want to test that it really fixes the problem and I have spent most of the time > finding out a way to trigger the timer in a race with the disconnect path. Sounds good, thanks a lot for working on this! > And that same test has showed me that this timer UAF will happen regardless of > commit 2677d20677314101293e6da0094ede7b5526d2b1, which led me into stating that > reverting it should be done in any case. > > I think I can find some time this week to work a little further on the fix for > the time UAF.
diff --git a/include/linux/dccp.h b/include/linux/dccp.h index 07e547c02fd8..504afa1a4be6 100644 --- a/include/linux/dccp.h +++ b/include/linux/dccp.h @@ -259,6 +259,7 @@ struct dccp_ackvec; * @dccps_sync_scheduled - flag which signals "send out-of-band message soon" * @dccps_xmitlet - tasklet scheduled by the TX CCID to dequeue data packets * @dccps_xmit_timer - used by the TX CCID to delay sending (rate-based pacing) + * @dccps_ccid_timer - used by the CCIDs * @dccps_syn_rtt - RTT sample from Request/Response exchange (in usecs) */ struct dccp_sock { @@ -303,6 +304,7 @@ struct dccp_sock { __u8 dccps_sync_scheduled:1; struct tasklet_struct dccps_xmitlet; struct timer_list dccps_xmit_timer; + struct timer_list dccps_ccid_timer; }; static inline struct dccp_sock *dccp_sk(const struct sock *sk) diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c index 3da1f77bd039..dbca1f1e2449 100644 --- a/net/dccp/ccids/ccid2.c +++ b/net/dccp/ccids/ccid2.c @@ -126,21 +126,26 @@ static void dccp_tasklet_schedule(struct sock *sk) static void ccid2_hc_tx_rto_expire(struct timer_list *t) { - struct ccid2_hc_tx_sock *hc = from_timer(hc, t, tx_rtotimer); - struct sock *sk = hc->sk; - const bool sender_was_blocked = ccid2_cwnd_network_limited(hc); + struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer); + struct sock *sk = (struct sock *)dp; + struct ccid2_hc_tx_sock *hc; + bool sender_was_blocked; bh_lock_sock(sk); + + if (inet_sk_state_load(sk) == DCCP_CLOSED) + goto out; + + hc = ccid_priv(dp->dccps_hc_tx_ccid); + sender_was_blocked = ccid2_cwnd_network_limited(hc); + if (sock_owned_by_user(sk)) { - sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + HZ / 5); + sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + HZ / 5); goto out; } ccid2_pr_debug("RTO_EXPIRE\n"); - if (sk->sk_state == DCCP_CLOSED) - goto out; - /* back-off timer */ hc->tx_rto <<= 1; if (hc->tx_rto > DCCP_RTO_MAX) @@ -166,7 +171,7 @@ static void ccid2_hc_tx_rto_expire(struct timer_list *t) if (sender_was_blocked) dccp_tasklet_schedule(sk); /* restart backed-off timer */ - sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto); + sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto); out: bh_unlock_sock(sk); sock_put(sk); @@ -330,7 +335,7 @@ static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len) } #endif - sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto); + sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto); #ifdef CONFIG_IP_DCCP_CCID2_DEBUG do { @@ -700,9 +705,9 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) /* restart RTO timer if not all outstanding data has been acked */ if (hc->tx_pipe == 0) - sk_stop_timer(sk, &hc->tx_rtotimer); + sk_stop_timer(sk, &dp->dccps_ccid_timer); else - sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto); + sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto); done: /* check if incoming Acks allow pending packets to be sent */ if (sender_was_blocked && !ccid2_cwnd_network_limited(hc)) @@ -737,17 +742,18 @@ static int ccid2_hc_tx_init(struct ccid *ccid, struct sock *sk) hc->tx_last_cong = hc->tx_lsndtime = hc->tx_cwnd_stamp = ccid2_jiffies32; hc->tx_cwnd_used = 0; hc->sk = sk; - timer_setup(&hc->tx_rtotimer, ccid2_hc_tx_rto_expire, 0); + timer_setup(&dp->dccps_ccid_timer, ccid2_hc_tx_rto_expire, 0); INIT_LIST_HEAD(&hc->tx_av_chunks); return 0; } static void ccid2_hc_tx_exit(struct sock *sk) { + struct dccp_sock *dp = dccp_sk(sk); struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk); int i; - sk_stop_timer(sk, &hc->tx_rtotimer); + sk_stop_timer(sk, &dp->dccps_ccid_timer); for (i = 0; i < hc->tx_seqbufc; i++) kfree(hc->tx_seqbuf[i]); diff --git a/net/dccp/ccids/ccid3.c b/net/dccp/ccids/ccid3.c index b9ee1a4a8955..685f4d046c0d 100644 --- a/net/dccp/ccids/ccid3.c +++ b/net/dccp/ccids/ccid3.c @@ -184,17 +184,24 @@ static inline void ccid3_hc_tx_update_win_count(struct ccid3_hc_tx_sock *hc, static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t) { - struct ccid3_hc_tx_sock *hc = from_timer(hc, t, tx_no_feedback_timer); - struct sock *sk = hc->sk; + struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer); + struct ccid3_hc_tx_sock *hc; + struct sock *sk = (struct sock *)dp; unsigned long t_nfb = USEC_PER_SEC / 5; bh_lock_sock(sk); + + if (inet_sk_state_load(sk) == DCCP_CLOSED) + goto out; + if (sock_owned_by_user(sk)) { /* Try again later. */ /* XXX: set some sensible MIB */ goto restart_timer; } + hc = ccid_priv(dp->dccps_hc_tx_ccid); + ccid3_pr_debug("%s(%p, state=%s) - entry\n", dccp_role(sk), sk, ccid3_tx_state_name(hc->tx_state)); @@ -250,8 +257,8 @@ static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t) t_nfb = max(hc->tx_t_rto, 2 * hc->tx_t_ipi); restart_timer: - sk_reset_timer(sk, &hc->tx_no_feedback_timer, - jiffies + usecs_to_jiffies(t_nfb)); + sk_reset_timer(sk, &dp->dccps_ccid_timer, + jiffies + usecs_to_jiffies(t_nfb)); out: bh_unlock_sock(sk); sock_put(sk); @@ -280,7 +287,7 @@ static int ccid3_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb) return -EBADMSG; if (hc->tx_state == TFRC_SSTATE_NO_SENT) { - sk_reset_timer(sk, &hc->tx_no_feedback_timer, (jiffies + + sk_reset_timer(sk, &dp->dccps_ccid_timer, (jiffies + usecs_to_jiffies(TFRC_INITIAL_TIMEOUT))); hc->tx_last_win_count = 0; hc->tx_t_last_win_count = now; @@ -354,6 +361,7 @@ static void ccid3_hc_tx_packet_sent(struct sock *sk, unsigned int len) static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) { struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk); + struct dccp_sock *dp = dccp_sk(sk); struct tfrc_tx_hist_entry *acked; ktime_t now; unsigned long t_nfb; @@ -420,7 +428,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) (unsigned int)(hc->tx_x >> 6)); /* unschedule no feedback timer */ - sk_stop_timer(sk, &hc->tx_no_feedback_timer); + sk_stop_timer(sk, &dp->dccps_ccid_timer); /* * As we have calculated new ipi, delta, t_nom it is possible @@ -445,8 +453,8 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) "expire in %lu jiffies (%luus)\n", dccp_role(sk), sk, usecs_to_jiffies(t_nfb), t_nfb); - sk_reset_timer(sk, &hc->tx_no_feedback_timer, - jiffies + usecs_to_jiffies(t_nfb)); + sk_reset_timer(sk, &dp->dccps_ccid_timer, + jiffies + usecs_to_jiffies(t_nfb)); } static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type, @@ -488,21 +496,23 @@ static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type, static int ccid3_hc_tx_init(struct ccid *ccid, struct sock *sk) { + struct dccp_sock *dp = dccp_sk(sk); struct ccid3_hc_tx_sock *hc = ccid_priv(ccid); hc->tx_state = TFRC_SSTATE_NO_SENT; hc->tx_hist = NULL; hc->sk = sk; - timer_setup(&hc->tx_no_feedback_timer, + timer_setup(&dp->dccps_ccid_timer, ccid3_hc_tx_no_feedback_timer, 0); return 0; } static void ccid3_hc_tx_exit(struct sock *sk) { + struct dccp_sock *dp = dccp_sk(sk); struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk); - sk_stop_timer(sk, &hc->tx_no_feedback_timer); + sk_stop_timer(sk, &dp->dccps_ccid_timer); tfrc_tx_hist_purge(&hc->tx_hist); }