On Mon, Apr 15, 2019 at 04:33:00PM +0800, xiao ruizhu wrote:
[...]
> When conntracks change during a dialog, SDP messages may be sent from
> different conntracks to establish expects with identical tuples. In this
> case expects conflict may be detected for the 2nd SDP message and end up
> with a process failure.
>
> The fixing here is to check both RTP and RTCP expect existence before
> creation. When there is an existing expect with the same tuples for a
> different conntrack, reuse it.
>
> Here are two scenarios for the case.
>
> 1)
> SERVER CPE
>
> | INVITE SDP |
> 5060 |<----------------------|5060
> | 100 Trying |
> 5060 |---------------------->|5060
> | 183 SDP |
> 5060 |---------------------->|5060 ===> Conntrack 1
> | PRACK |
> 50601 |<----------------------|5060
> | 200 OK (PRACK) |
> 50601 |---------------------->|5060
> | 200 OK (INVITE) |
> 5060 |---------------------->|5060
> | ACK |
> 50601 |<----------------------|5060
> | |
> |<--- RTP stream ------>|
> | |
> | INVITE SDP (t38) |
> 50601 |---------------------->|5060 ===> Conntrack 2
>
> With a certain configuration in the CPE, SIP messages "183 with SDP" and
> "re-INVITE with SDP t38" will go through the sip helper to create
> expects for RTP and RTCP.
>
> It is okay to create RTP and RTCP expects for "183", whose master
> connection source port is 5060, and destination port is 5060.
>
> In the "183" message, port in Contact header changes to 50601 (from the
> original 5060). So the following requests e.g. PRACK and ACK are sent to
> port 50601. It is a different conntrack (let call Conntrack 2) from the
> original INVITE (let call Conntrack 1) due to the port difference.
>
> In this example, after the call is established, there is RTP stream but no
> RTCP stream for Conntrack 1, so the RTP expect created upon "183" is
> cleared, and RTCP expect created for Conntrack 1 retains.
>
> When "re-INVITE with SDP t38" arrives to create RTP&RTCP expects, current
> ALG implementation will call nf_ct_expect_related() for RTP and RTCP. The
> expects tuples are identical to those for Conntrack 1. RTP expect for
> Conntrack 2 succeeds in creation as the one for Conntrack 1 has been
> removed. RTCP expect for Conntrack 2 fails in creation because it has
> idential tuples and 'conflict' with the one retained for Conntrack 1. And
> then result in a failure in processing of the re-INVITE.
>
> 2)
>
> SERVER A CPE
>
> | REGISTER |
> 5060 |<------------------| 5060 ==> CT1
> | 200 |
> 5060 |------------------>| 5060
> | |
> | INVITE SDP(1) |
> 5060 |<------------------| 5060
> | 300(multi choice) |
> 5060 |------------------>| 5060 SERVER B
> | ACK |
> 5060 |<------------------| 5060
> | INVITE SDP(2) |
> 5060 |-------------------->| 5060 ==> CT2
> | 100 |
> 5060 |<--------------------| 5060
> | 200(contact changes)|
> 5060 |<--------------------| 5060
> | ACK |
> 5060 |-------------------->| 50601 ==> CT3
> | |
> |<--- RTP stream ---->|
> | |
> | BYE |
> 5060 |<--------------------| 50601
> | 200 |
> 5060 |-------------------->| 50601
> | INVITE SDP(3) |
> 5060 |<------------------| 5060 ==> CT1
>
> CPE sends an INVITE request(1) to Server A, and creates a RTP&RTCP expect
> pair for this Conntrack 1 (CT1). Server A responds 300 to redirect to
> Server B. The RTP&RTCP expect pairs created on CT1 are removed upon 300
> response.
>
> CPE sends the INVITE request(2) to Server B, and creates an expect pair
> for the new conntrack (due to destination address difference), let call
> CT2. Server B changes the port to 50601 in 200 OK response, and the
> following requests ACK and BYE from CPE are sent to 50601. The call is
> established. There is RTP stream and no RTCP stream. So RTP expect is
> removed and RTCP expect for CT2 retains.
>
> As BYE request is sent from port 50601, it is another conntrack, let call
> CT3, different from CT2 due to the port difference. So the BYE request will
> not remove the RTCP expect for CT2.
>
> Then another outgoing call is made, with the same RTP port being used (not
> definitely but possibly). CPE firstly sends the INVITE request(3) to Server
> A, and tries to create a RTP&RTCP expect pairs for this CT1. In current ALG
> implementation, the RTCP expect for CT1 fails in creation because it
> 'conflicts' with the residual one for CT2. As a result the INVITE request
> fails to send.
>
> Signed-off-by: xiao ruizhu <katrina.xiaorz@xxxxxxxxx>
> ---
> Changes in v3:
> - take Pablo's advice about the comments, nf_conntrack_expect_lock and
> nf_ct_sip_expect_exists()
> - change the policy to reuse the exising expect(s) instead of removal then
> recreation, to avoid CPU cycle waste
> 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 | 45 +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index f067c6b..0e17c14 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -799,6 +799,31 @@ static int ct_sip_parse_sdp_addr(const struct nf_conn *ct, const char *dptr,
> return 1;
> }
>
> +static bool nf_ct_sip_expect_exists(const struct nf_conntrack_expect *expect,
> + const struct nf_conn *ct,
> + enum sip_expectation_classes class)
> +{
> + return (expect && expect->master != ct &&
> + nfct_help(expect->master)->helper == nfct_help(ct)->helper &&
> + expect->class == class);
> +}
> +
> +/* Look for an expect with identical tuple but for a different master. */
> +static bool nf_ct_sip_expect_found(const struct nf_conntrack_expect *expect,
> + const struct nf_conn *ct)
> +{
> + struct nf_conntrack_expect *exp;
> + struct net *net = nf_ct_net(ct);
> + bool found = 0;
> +
> + spin_lock_bh(&nf_conntrack_expect_lock);
> + exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &expect->tuple);
__nf_ct_expect_find() may return NULL.
> + found = nf_ct_sip_expect_exists(exp, ct, expect->class);
> + spin_unlock_bh(&nf_conntrack_expect_lock);
> +
> + return found;
> +}
> +
> static int refresh_signalling_expectation(struct nf_conn *ct,
> union nf_inet_addr *addr,
> u8 proto, __be16 port,
> @@ -929,9 +954,7 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
> do {
> exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &tuple);
>
> - if (!exp || exp->master == ct ||
> - nfct_help(exp->master)->helper != nfct_help(ct)->helper ||
> - exp->class != class)
> + if (!nf_ct_sip_expect_exists(exp, ct, class))
> break;
> #ifdef CONFIG_NF_NAT_NEEDED
> if (!direct_rtp &&
> @@ -983,11 +1006,23 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
> /* -EALREADY handling works around end-points that send
> * SDP messages with identical port but different media type,
> * we pretend expectation was set up.
> + * It also works in the case that SDP messages are sent with
> + * identical expect tuples but for different master conntracks.
> */
> - int errp = nf_ct_expect_related(rtp_exp);
> + int errp;
> +
> + if (nf_ct_sip_expect_found(rtp_exp, ct))
> + errp = -EALREADY;
> + else
> + errp = nf_ct_expect_related(rtp_exp);
I wonder if we can handle this from __nf_ct_expect_check() itself.
We could just check if master mismatches, then return -EALREADY from
there?
Similar to 876c27314ce51, but catch the master mismatches case.
> if (errp == 0 || errp == -EALREADY) {
> - int errcp = nf_ct_expect_related(rtcp_exp);
> + int errcp;
> +
> + if (nf_ct_sip_expect_found(rtcp_exp, ct))
> + errcp = -EALREADY;
> + else
> + errcp = nf_ct_expect_related(rtcp_exp);
>
> if (errcp == 0 || errcp == -EALREADY)
> ret = NF_ACCEPT;
> --
> 1.9.1
>
