| Message ID | 20130920155817.660882995@eitzenberger.org |
|---|---|
| State | Superseded |
| Headers | show |
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
> > 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
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; }
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