Patchwork [FIX,1/1] sip: add missing RCU reader lock

login
register
mail settings
Submitter holger@eitzenberger.org
Date Sept. 20, 2013, 3:52 p.m.
Message ID <20130920155817.660882995@eitzenberger.org>
Download mbox | patch
Permalink /patch/276522/
State Superseded
Headers show

Comments

holger@eitzenberger.org - Sept. 20, 2013, 3:52 p.m.
Currently set_expected_rtp_rtcp() in the SIP helper uses
rcu_dereference() two times to access two different NAT hook
functions.  However, only the first one is protected properly by
the RCU reader lock, but the 2nd isn't.

I chose to not just extend the first RCU protected area but putting
the rcu_read_unlock() down, because there is a 'return' in between.

Signed-off-by: Holger Eitzenberger <holger.eitzenberger@sophos.com>


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy - Sept. 20, 2013, 4:55 p.m.
On Fri, Sep 20, 2013 at 05:52:18PM +0200, Holger Eitzenberger wrote:
> Currently set_expected_rtp_rtcp() in the SIP helper uses
> rcu_dereference() two times to access two different NAT hook
> functions.  However, only the first one is protected properly by
> the RCU reader lock, but the 2nd isn't.

Its more a cosmetic thing since we rely on all netfilter hooks being
rcu_read_lock()ed by nf_hook_slow() in many places anyways.

> I chose to not just extend the first RCU protected area but putting
> the rcu_read_unlock() down, because there is a 'return' in between.

I'd suggest to do that since your patch still doesn't cover the
direct_rtp nf_nat_sdp_port_hook dereference. Actually with your patch
we have unbalanced RCU locking since the direct_rtp case contains
a "goto err1".

> 
> Signed-off-by: Holger Eitzenberger <holger.eitzenberger@sophos.com>
> 
> Index: net-next/net/netfilter/nf_conntrack_sip.c
> ===================================================================
> --- net-next.orig/net/netfilter/nf_conntrack_sip.c
> +++ net-next/net/netfilter/nf_conntrack_sip.c
> @@ -983,6 +983,7 @@ static int set_expected_rtp_rtcp(struct
>  	if (skip_expect)
>  		return NF_ACCEPT;
>  
> +	rcu_read_lock();
>  	rtp_exp = nf_ct_expect_alloc(ct);
>  	if (rtp_exp == NULL)
>  		goto err1;
> @@ -1012,6 +1013,7 @@ static int set_expected_rtp_rtcp(struct
>  err2:
>  	nf_ct_expect_put(rtp_exp);
>  err1:
> +	rcu_read_unlock();
>  	return ret;
>  }
>  
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
holger@eitzenberger.org - Sept. 20, 2013, 8:20 p.m.
> > I chose to not just extend the first RCU protected area but putting
> > the rcu_read_unlock() down, because there is a 'return' in between.
> 
> I'd suggest to do that since your patch still doesn't cover the
> direct_rtp nf_nat_sdp_port_hook dereference. Actually with your patch
> we have unbalanced RCU locking since the direct_rtp case contains
> a "goto err1".

Sure, I'll send a 2nd patch in a new thread.

 /Holger

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

Patch

Index: net-next/net/netfilter/nf_conntrack_sip.c
===================================================================
--- net-next.orig/net/netfilter/nf_conntrack_sip.c
+++ net-next/net/netfilter/nf_conntrack_sip.c
@@ -983,6 +983,7 @@  static int set_expected_rtp_rtcp(struct
 	if (skip_expect)
 		return NF_ACCEPT;
 
+	rcu_read_lock();
 	rtp_exp = nf_ct_expect_alloc(ct);
 	if (rtp_exp == NULL)
 		goto err1;
@@ -1012,6 +1013,7 @@  static int set_expected_rtp_rtcp(struct
 err2:
 	nf_ct_expect_put(rtp_exp);
 err1:
+	rcu_read_unlock();
 	return ret;
 }