| Message ID | 1552534124-31407-1-git-send-email-katrina.xiaorz@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash | expand |
On Thu, Mar 14, 2019 at 4:27 AM xiao ruizhu <katrina.xiaorz@gmail.com> wrote: > > An unexpected RTCP expectation clash happens in the following scenario. > > | INVITE SDP (g711A g739 telephone-event) | > 5060 |<--------------------------------------------------|5060 > | 100 Trying | > 5060 |-------------------------------------------------->|5060 > S | 183 Session Progress SDP (g711A telephone-event) | > 5060 |-------------------------------------------------->|5060 > E | PRACK | C > 50601 |<--------------------------------------------------|5060 > R | 200 OK (PRACK) | P > 50601 |-------------------------------------------------->|5060 > V | 200 OK (INVITE) | E > 5060 |-------------------------------------------------->|5060 > E | ACK | > 50601 |<--------------------------------------------------|5060 > R | | > |<------------------ RTP stream ------------------->| > | | > | INVITE SDP (t38) | > 50601 |-------------------------------------------------->|5060 > > With a certain configuration in the CPE, SIP messages "183 with SDP" and > "re-INVITE with SDP t38" in the scenario will trigger the ALG to create > expectations for RTP and RTCP. > > It is okay to create RTP&RTCP expectations for "183", whose master > connection source port is 5060, and destination port is 5060. > > In this "183" message, port in Contact header changes to 50601 (from the > original 5060). So the following requests e.g. PRACK are sent to port > 50601. It has a different master connection (let call 2nd master > connection) from the original INVITE (let call 1st master connection) due > to the port difference. > > When "re-INVITE with SDP t38" arrives to create RTP&RTCP expectations, > current ALG implementation will firstly check whether there is a RTP > expectation with the same tuples already exists for a different master > connection. If yes, it will skip RTP&RTCP expects creation; otherwise it > will create a new RTP&RTCP expectation pairs. > > In the scenario above, there is RTP stream but no RTCP stream for the 1st > master connection, so the RTP expectation created upon "183" is cleared, > and RTCP expectation created for the 1st master connection retains. > > Basing on current ALG implementation, it only checks RTP expectation > existence but not for RTCP. So when "re-INVITE with SDP t38" arrives, it > will continue to create a new RTP&RTCP expectation pairs for the 2nd master > connection, which will result in a detection of expectation clash for RTCP > (same tuples and different master), and then a failure in processing of > that re-INVITE. > > The fixing here is to check both RTP and RTCP expectation existence before > creation. When there is an old expectation with same tuples for a different > master, release the old expectation. Then a new one will be created for the > current master. > > Signed-off-by: xiao ruizhu <katrina.xiaorz@gmail.com> > --- > Changes in v2: > - add a comment on release_conflicting_expect functionality > - move local variable errp to the beginning of the block > v1: > - original patch > --- > net/netfilter/nf_conntrack_sip.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c > index f067c6b..4398a53 100644 > --- a/net/netfilter/nf_conntrack_sip.c > +++ b/net/netfilter/nf_conntrack_sip.c > @@ -799,6 +799,23 @@ static int ct_sip_parse_sdp_addr(const struct nf_conn *ct, const char *dptr, > return 1; > } > > +/* Remove conflicting expect created by sip helper for another master > + * conntrack, most likely related to this master. > + */ > +static void release_conflicting_expect(const struct nf_conn *ct, > + const struct nf_conntrack_expect *expect, > + const enum sip_expectation_classes class) > +{ > + struct nf_conntrack_expect *exp; > + struct net *net = nf_ct_net(ct); > + > + exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &expect->tuple); > + if (exp && exp->master != ct && > + nfct_help(exp->master)->helper == nfct_help(ct)->helper && > + exp->class == class) > + nf_ct_unexpect_related(exp); > +} > + > static int refresh_signalling_expectation(struct nf_conn *ct, > union nf_inet_addr *addr, > u8 proto, __be16 port, > @@ -980,11 +997,16 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff, > datalen, rtp_exp, rtcp_exp, > mediaoff, medialen, daddr); > else { > + int errp; > + > + release_conflicting_expect(ct, rtp_exp, class); > + release_conflicting_expect(ct, rtcp_exp, class); > + > /* -EALREADY handling works around end-points that send > * SDP messages with identical port but different media type, > * we pretend expectation was set up. > */ > - int errp = nf_ct_expect_related(rtp_exp); > + errp = nf_ct_expect_related(rtp_exp); > > if (errp == 0 || errp == -EALREADY) { > int errcp = nf_ct_expect_related(rtcp_exp); > -- > 1.9.1 > Acked-by: Alin Nastac <alin.nastac@gmail.com>
Hi, On Thu, Mar 14, 2019 at 11:28:44AM +0800, xiao ruizhu wrote: > diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c > index f067c6b..4398a53 100644 > --- a/net/netfilter/nf_conntrack_sip.c > +++ b/net/netfilter/nf_conntrack_sip.c > @@ -799,6 +799,23 @@ static int ct_sip_parse_sdp_addr(const struct nf_conn *ct, const char *dptr, > return 1; > } > > +/* Remove conflicting expect created by sip helper for another master > + * conntrack, most likely related to this master. Hm, this comment is confusing. Code here that checks that master is not the same, however, this suggests this is likely the same master, however, it is related to a different master? More comments related to the code changes below. > + */ > +static void release_conflicting_expect(const struct nf_conn *ct, > + const struct nf_conntrack_expect *expect, > + const enum sip_expectation_classes class) > +{ > + struct nf_conntrack_expect *exp; > + struct net *net = nf_ct_net(ct); > + We need to hold the nf_conntrack_expect_lock here, two CPUs may race to destroy this expectation, unlikely but possible still. > + exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &expect->tuple); > + if (exp && exp->master != ct && > + nfct_help(exp->master)->helper == nfct_help(ct)->helper && > + exp->class == class) I'd suggest you place this into a funcion that we can reuse from set_expected_rtp_rtcp(), eg. static bool nf_ct_sip_expect_exists(const struct nf_conntrack_expect *expect, const struct nf_conn *ct) { return (exp && exp->master != ct && nfct_help(exp->master)->helper == nfct_help(ct)->helper && exp->class == class); } So we can reuse it from L927 in set_expected_rtp_rtcp()? > + nf_ct_unexpect_related(exp); Use nf_ct_remove_expect() instead of nf_ct_unexpect_related(), while holding the spinlock. And a more general question: Given the scenario you describe is quite specific, is it not possible to handle this case from process_sip_response() -> process_prack_response(). Thanks !
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c index f067c6b..4398a53 100644 --- a/net/netfilter/nf_conntrack_sip.c +++ b/net/netfilter/nf_conntrack_sip.c @@ -799,6 +799,23 @@ static int ct_sip_parse_sdp_addr(const struct nf_conn *ct, const char *dptr, return 1; } +/* Remove conflicting expect created by sip helper for another master + * conntrack, most likely related to this master. + */ +static void release_conflicting_expect(const struct nf_conn *ct, + const struct nf_conntrack_expect *expect, + const enum sip_expectation_classes class) +{ + struct nf_conntrack_expect *exp; + struct net *net = nf_ct_net(ct); + + exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &expect->tuple); + if (exp && exp->master != ct && + nfct_help(exp->master)->helper == nfct_help(ct)->helper && + exp->class == class) + nf_ct_unexpect_related(exp); +} + static int refresh_signalling_expectation(struct nf_conn *ct, union nf_inet_addr *addr, u8 proto, __be16 port, @@ -980,11 +997,16 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff, datalen, rtp_exp, rtcp_exp, mediaoff, medialen, daddr); else { + int errp; + + release_conflicting_expect(ct, rtp_exp, class); + release_conflicting_expect(ct, rtcp_exp, class); + /* -EALREADY handling works around end-points that send * SDP messages with identical port but different media type, * we pretend expectation was set up. */ - int errp = nf_ct_expect_related(rtp_exp); + errp = nf_ct_expect_related(rtp_exp); if (errp == 0 || errp == -EALREADY) { int errcp = nf_ct_expect_related(rtcp_exp);
An unexpected RTCP expectation clash happens in the following scenario. | INVITE SDP (g711A g739 telephone-event) | 5060 |<--------------------------------------------------|5060 | 100 Trying | 5060 |-------------------------------------------------->|5060 S | 183 Session Progress SDP (g711A telephone-event) | 5060 |-------------------------------------------------->|5060 E | PRACK | C 50601 |<--------------------------------------------------|5060 R | 200 OK (PRACK) | P 50601 |-------------------------------------------------->|5060 V | 200 OK (INVITE) | E 5060 |-------------------------------------------------->|5060 E | ACK | 50601 |<--------------------------------------------------|5060 R | | |<------------------ RTP stream ------------------->| | | | INVITE SDP (t38) | 50601 |-------------------------------------------------->|5060 With a certain configuration in the CPE, SIP messages "183 with SDP" and "re-INVITE with SDP t38" in the scenario will trigger the ALG to create expectations for RTP and RTCP. It is okay to create RTP&RTCP expectations for "183", whose master connection source port is 5060, and destination port is 5060. In this "183" message, port in Contact header changes to 50601 (from the original 5060). So the following requests e.g. PRACK are sent to port 50601. It has a different master connection (let call 2nd master connection) from the original INVITE (let call 1st master connection) due to the port difference. When "re-INVITE with SDP t38" arrives to create RTP&RTCP expectations, current ALG implementation will firstly check whether there is a RTP expectation with the same tuples already exists for a different master connection. If yes, it will skip RTP&RTCP expects creation; otherwise it will create a new RTP&RTCP expectation pairs. In the scenario above, there is RTP stream but no RTCP stream for the 1st master connection, so the RTP expectation created upon "183" is cleared, and RTCP expectation created for the 1st master connection retains. Basing on current ALG implementation, it only checks RTP expectation existence but not for RTCP. So when "re-INVITE with SDP t38" arrives, it will continue to create a new RTP&RTCP expectation pairs for the 2nd master connection, which will result in a detection of expectation clash for RTCP (same tuples and different master), and then a failure in processing of that re-INVITE. The fixing here is to check both RTP and RTCP expectation existence before creation. When there is an old expectation with same tuples for a different master, release the old expectation. Then a new one will be created for the current master. Signed-off-by: xiao ruizhu <katrina.xiaorz@gmail.com> --- Changes in v2: - add a comment on release_conflicting_expect functionality - move local variable errp to the beginning of the block v1: - original patch --- net/netfilter/nf_conntrack_sip.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)