* [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash
@ 2019-03-14 3:28 xiao ruizhu
2019-03-14 8:21 ` Alin Năstac
2019-03-21 10:56 ` Pablo Neira Ayuso
0 siblings, 2 replies; 15+ messages in thread
From: xiao ruizhu @ 2019-03-14 3:28 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, netfilter-devel; +Cc: xiao ruizhu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash
2019-03-14 3:28 [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash xiao ruizhu
@ 2019-03-14 8:21 ` Alin Năstac
2019-03-21 10:56 ` Pablo Neira Ayuso
1 sibling, 0 replies; 15+ messages in thread
From: Alin Năstac @ 2019-03-14 8:21 UTC (permalink / raw)
To: xiao ruizhu
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David Miller, netfilter-devel
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash
2019-03-14 3:28 [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash xiao ruizhu
2019-03-14 8:21 ` Alin Năstac
@ 2019-03-21 10:56 ` Pablo Neira Ayuso
2019-04-15 8:33 ` [PATCH v3] netfilter: nf_conntrack_sip: fix " xiao ruizhu
1 sibling, 1 reply; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-03-21 10:56 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, netfilter-devel
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 !
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] netfilter: nf_conntrack_sip: fix expectation clash
2019-03-21 10:56 ` Pablo Neira Ayuso
@ 2019-04-15 8:33 ` xiao ruizhu
2019-05-13 11:26 ` Pablo Neira Ayuso
0 siblings, 1 reply; 15+ messages in thread
From: xiao ruizhu @ 2019-04-15 8:33 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
Hi,
Thanks a lot for your feedback.
There are some changes in the implementation and please find my answers and
the patch v3 below.
>
>
>On Thu, Mar 21, 2019 at 06:56PM, Pablo Neira Ayuso <pablo@netfilter.org>
>wrote:
>Hi,
>...
>> +/* 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?
Yes, I meant it is related to a different master. The function and comments
are changed and please find them in the new patch below.
>
>We need to hold the nf_conntrack_expect_lock here, two CPUs may race
>to destroy this expectation, unlikely but possible still.
Thanks for the reminding. Ok, will hold nf_conntrack_expect_lock as
proposed.
>
>> + 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 function 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()?
Ok, it will be placed into a function as proposed.
>
>> + nf_ct_unexpect_related(exp);
>
>Use nf_ct_remove_expect() instead of nf_ct_unexpect_related(), while
>holding the spinlock.
The implementation changes to reuse the expect but not to remove and
recreate, so the call here will be removed.
>
>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().
>
In this case the issue happens no matter there is PRACK or not.
Actually, this issue may happen when the master conntrack changes during a
dialog. So I think it is more of a generic issue and would be better to
keep the handling in set_expected_rtp_rtcp().
Please find the details blow. Thanks.
---
Hereunder is the patch v3
---
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@gmail.com>
---
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);
+ 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);
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] netfilter: nf_conntrack_sip: fix expectation clash
2019-04-15 8:33 ` [PATCH v3] netfilter: nf_conntrack_sip: fix " xiao ruizhu
@ 2019-05-13 11:26 ` Pablo Neira Ayuso
[not found] ` <CAEorUYZe2mtLupMDkAOvMXZoH_NcUOKLR=K4atLC5dddHOs-MQ@mail.gmail.com>
0 siblings, 1 reply; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-13 11:26 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, alin.nastac, netfilter-devel
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@gmail.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] netfilter: nf_conntrack_sip: fix expectation clash
[not found] ` <CAEorUYZe2mtLupMDkAOvMXZoH_NcUOKLR=K4atLC5dddHOs-MQ@mail.gmail.com>
@ 2019-05-14 10:18 ` Pablo Neira Ayuso
2019-05-15 7:45 ` [PATCH v4] " xiao ruizhu
2019-06-04 8:34 ` [PATCH v5] " xiao ruizhu
0 siblings, 2 replies; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-14 10:18 UTC (permalink / raw)
To: 肖瑞珠
Cc: Jozsef Kadlecsik, Florian Westphal, davem, alin.nastac, netfilter-devel
Hi Xiao,
On Tue, May 14, 2019 at 03:45:13PM +0800, 肖瑞珠 wrote:
> Hi Pablo,
>
> Thanks very much for your reply.
>
> >On Thu, May 13, 2019 at 07:26PM, Pablo Neira Ayuso <pablo@netfilter.org>
> >wrote:
> >
> >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.
>
> Thanks for your proposal. It is a neater solution.
OK, thanks for exploring this path and confirming this works!
Still one more question before we go: I wonder if we should enable
this through flag, eg. extend nf_ct_expect_related() to take a flag
that NFCT_EXP_F_MASTER_MISMATCH.
This would change the behaviour for the other existing helpers, which
would prevent them from creating expectations with the same tuple from
different master conntracks.
So I would just turn on this for SIP unless there is some reasoning
here that turning it for all existing helpers is fine.
One more comment below.
> Please find the patch updated accordingly below.
For some reason this patch is not showing in patchwork:
https://patchwork.ozlabs.org/project/netfilter-devel/list/
Would you resubmit via git send-mail?
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] netfilter: nf_conntrack_sip: fix expectation clash
2019-05-14 10:18 ` Pablo Neira Ayuso
@ 2019-05-15 7:45 ` xiao ruizhu
2019-06-04 8:34 ` [PATCH v5] " xiao ruizhu
1 sibling, 0 replies; 15+ messages in thread
From: xiao ruizhu @ 2019-05-15 7:45 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
Hi Pablo,
Please find my comments below.
> On Thu, May 14, 2019 at 06:18PM, Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
> Hi Xiao,
>
> On Tue, May 14, 2019 at 03:45:13PM +0800, 肖瑞珠 wrote:
> > Hi Pablo,
> >
> > Thanks very much for your reply.
> >
> > >On Thu, May 13, 2019 at 07:26PM, Pablo Neira Ayuso
> > ><pablo@netfilter.org> wrote:
> > >
> > >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.
> >
> > Thanks for your proposal. It is a neater solution.
> OK, thanks for exploring this path and confirming this works!
>
> Still one more question before we go: I wonder if we should enable
> this through flag, eg. extend nf_ct_expect_related() to take a flag
> that NFCT_EXP_F_MASTER_MISMATCH.
>
> This would change the behaviour for the other existing helpers, which
> would prevent them from creating expectations with the same tuple from
> different master conntracks.
>
> So I would just turn on this for SIP unless there is some reasoning
> here that turning it for all existing helpers is fine.
Yes, please feel free to add the flag NFCT_EXP_F_MASTER_MISMATCH. It will
minimize the impact on other helpers. Thanks.
>
> One more comment below.
>
> > Please find the patch updated accordingly below.
>
> For some reason this patch is not showing in patchwork:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/list/
>
> Would you resubmit via git send-mail?
>
> Thanks.
Sorry, the network for using 'git send-mail' broke and the mail was sent
via gui web.
This time it is submitted via git send-mail. Please don't hesitate to let
me know if still any problem.
Please find the patch v4 below.
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 reuse an existing expect who has the same tuple for a
different conntrack if any.
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@gmail.com>
---
Changes in v4:
- take Pablo's proposal to handle checking in __nf_ct_expect_check().
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_expect.c | 6 +++---
net/netfilter/nf_conntrack_sip.c | 2 ++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 334d6e5..bfc7936 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -252,8 +252,7 @@ static inline int expect_clash(const struct nf_conntrack_expect *a,
static inline int expect_matches(const struct nf_conntrack_expect *a,
const struct nf_conntrack_expect *b)
{
- return a->master == b->master &&
- nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
+ return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
@@ -421,7 +420,8 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
h = nf_ct_expect_dst_hash(net, &expect->tuple);
hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
if (expect_matches(i, expect)) {
- if (i->class != expect->class)
+ if (i->class != expect->class ||
+ i->master != expect->master)
return -EALREADY;
if (nf_ct_remove_expect(i))
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index f067c6b..3262fd9 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -983,6 +983,8 @@ 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);
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5] netfilter: nf_conntrack_sip: fix expectation clash
2019-05-14 10:18 ` Pablo Neira Ayuso
2019-05-15 7:45 ` [PATCH v4] " xiao ruizhu
@ 2019-06-04 8:34 ` xiao ruizhu
2019-06-10 17:45 ` Pablo Neira Ayuso
1 sibling, 1 reply; 15+ messages in thread
From: xiao ruizhu @ 2019-06-04 8:34 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
> On Thu, May 14, 2019 at 06:18PM, Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
> Hi Xiao,
>
> On Tue, May 14, 2019 at 03:45:13PM +0800, 肖瑞珠 wrote:
> > Hi Pablo,
> >
> > Thanks very much for your reply.
> >
> > >On Thu, May 13, 2019 at 07:26PM, Pablo Neira Ayuso
> > ><pablo@netfilter.org> wrote:
> > >
> > >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.
> >
> > Thanks for your proposal. It is a neater solution.
> OK, thanks for exploring this path and confirming this works!
>
> Still one more question before we go: I wonder if we should enable
> this through flag, eg. extend nf_ct_expect_related() to take a flag
> that NFCT_EXP_F_MASTER_MISMATCH.
>
> This would change the behaviour for the other existing helpers, which
> would prevent them from creating expectations with the same tuple from
> different master conntracks.
>
> So I would just turn on this for SIP unless there is some reasoning
> here that turning it for all existing helpers is fine.
Thanks for the proposal. The flag will minimize the impact on other
helpers. Codes are updated accordingly.
>
> One more comment below.
>
> > Please find the patch updated accordingly below.
>
> For some reason this patch is not showing in patchwork:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/list/
>
> Would you resubmit via git send-mail?
>
> Thanks.
Sure, the patch updated will be sent via git send-mail.
Please find the patch v5 below.
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 reuse an existing expect who has the same tuple for a
different conntrack if any.
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@gmail.com>
---
Changes in v5:
- take Pablo's proposal to use a flag in order not to change behavior of
the other existing helpers.
Changes in v4:
- take Pablo's proposal to handle checking in __nf_ct_expect_check().
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
---
include/net/netfilter/nf_conntrack_expect.h | 12 +++++++++---
net/ipv4/netfilter/nf_nat_h323.c | 12 ++++++------
net/netfilter/ipvs/ip_vs_nfct.c | 2 +-
net/netfilter/nf_conntrack_amanda.c | 2 +-
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_conntrack_expect.c | 13 ++++++++-----
net/netfilter/nf_conntrack_ftp.c | 2 +-
net/netfilter/nf_conntrack_h323_main.c | 18 +++++++++---------
net/netfilter/nf_conntrack_irc.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 4 ++--
net/netfilter/nf_conntrack_pptp.c | 4 ++--
net/netfilter/nf_conntrack_sane.c | 2 +-
net/netfilter/nf_conntrack_sip.c | 10 +++++++---
net/netfilter/nf_conntrack_tftp.c | 2 +-
net/netfilter/nf_nat_amanda.c | 2 +-
net/netfilter/nf_nat_ftp.c | 2 +-
net/netfilter/nf_nat_irc.c | 2 +-
net/netfilter/nf_nat_sip.c | 8 +++++---
net/netfilter/nf_nat_tftp.c | 2 +-
19 files changed, 59 insertions(+), 44 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 93ce6b0..6da901e 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -76,6 +76,11 @@ struct nf_conntrack_expect_policy {
#define NF_CT_EXPECT_CLASS_DEFAULT 0
#define NF_CT_EXPECT_MAX_CNT 255
+/* Allow to reuse expectations with the same tuples from different master
+ * conntracks.
+ */
+#define NF_CT_EXP_F_CHECK_MASTER 0x1
+
int nf_conntrack_expect_pernet_init(struct net *net);
void nf_conntrack_expect_pernet_fini(struct net *net);
@@ -122,10 +127,11 @@ void nf_ct_expect_init(struct nf_conntrack_expect *, unsigned int, u_int8_t,
u_int8_t, const __be16 *, const __be16 *);
void nf_ct_expect_put(struct nf_conntrack_expect *exp);
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report);
-static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect)
+ u32 portid, int report, unsigned int flags);
+static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
- return nf_ct_expect_related_report(expect, 0, 0);
+ return nf_ct_expect_related_report(expect, 0, 0, flags);
}
#endif /*_NF_CONNTRACK_EXPECT_H*/
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 7875c98..f06709a 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -222,11 +222,11 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(nated_port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp, 0);
if (ret == 0) {
rtcp_exp->tuple.dst.u.udp.port =
htons(nated_port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp, 0);
if (ret == 0)
break;
else if (ret == -EBUSY) {
@@ -297,7 +297,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -353,7 +353,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -445,7 +445,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -538,7 +538,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index eb8b9c8..45bb5a3 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -247,7 +247,7 @@ void ip_vs_nfct_expect_related(struct sk_buff *skb, struct nf_conn *ct,
IP_VS_DBG_BUF(7, "%s: ct=%p, expect tuple=" FMT_TUPLE "\n",
__func__, ct, ARG_TUPLE(&exp->tuple));
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
}
EXPORT_SYMBOL(ip_vs_nfct_expect_related);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index dbec6fc..5d2f72b 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -163,7 +163,7 @@ static int amanda_help(struct sk_buff *skb,
if (nf_nat_amanda && ct->status & IPS_NAT_MASK)
ret = nf_nat_amanda(skb, ctinfo, protoff,
off - dataoff, len, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 5423b19..1563c86 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -67,7 +67,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
exp->class = NF_CT_EXPECT_CLASS_DEFAULT;
exp->helper = NULL;
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
nf_ct_refresh(ct, skb, timeout * HZ);
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 59c1880..569f889 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -402,7 +402,8 @@ static void evict_oldest_expect(struct nf_conn *master,
nf_ct_remove_expect(last);
}
-static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
+static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
const struct nf_conntrack_expect_policy *p;
struct nf_conntrack_expect *i;
@@ -420,8 +421,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
h = nf_ct_expect_dst_hash(net, &expect->tuple);
hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
- if (expect_matches(i, expect)) {
- if (i->class != expect->class)
+ if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
+ expect->master) && expect_matches(i, expect)) {
+ if (i->class != expect->class ||
+ i->master != expect->master)
return -EALREADY;
if (nf_ct_remove_expect(i))
@@ -456,12 +459,12 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report)
+ u32 portid, int report, unsigned int flags)
{
int ret;
spin_lock_bh(&nf_conntrack_expect_lock);
- ret = __nf_ct_expect_check(expect);
+ ret = __nf_ct_expect_check(expect, flags);
if (ret < 0)
goto out;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 32aeac1..1578bfa 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -528,7 +528,7 @@ static int help(struct sk_buff *skb,
protoff, matchoff, matchlen, exp);
else {
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 12de403..207024c 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -306,8 +306,8 @@ static int expect_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_rtp_rtcp(skb, ct, ctinfo, protoff, data, dataoff,
taddr, port, rtp_port, rtp_exp, rtcp_exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(rtp_exp) == 0) {
- if (nf_ct_expect_related(rtcp_exp) == 0) {
+ if (nf_ct_expect_related(rtp_exp, 0) == 0) {
+ if (nf_ct_expect_related(rtcp_exp, 0) == 0) {
pr_debug("nf_ct_h323: expect RTP ");
nf_ct_dump_tuple(&rtp_exp->tuple);
pr_debug("nf_ct_h323: expect RTCP ");
@@ -365,7 +365,7 @@ static int expect_t120(struct sk_buff *skb,
ret = nat_t120(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_h323: expect T.120 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -702,7 +702,7 @@ static int expect_h245(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_h245(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect H.245 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -826,7 +826,7 @@ static int expect_callforwarding(struct sk_buff *skb,
protoff, data, dataoff,
taddr, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect Call Forwarding ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1285,7 +1285,7 @@ static int expect_q931(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_q931(skb, ct, ctinfo, protoff, data,
taddr, i, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
@@ -1350,7 +1350,7 @@ static int process_gcf(struct sk_buff *skb, struct nf_conn *ct,
IPPROTO_UDP, NULL, &port);
exp->helper = nf_conntrack_helper_ras;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect RAS ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1562,7 +1562,7 @@ static int process_acf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1616,7 +1616,7 @@ static int process_lcf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 79e5014..92a118f 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -217,7 +217,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
addr_beg_p - ib_ptr,
addr_end_p - addr_beg_p,
exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct,
"cannot add expectation");
ret = NF_DROP;
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7db79c1..56d3ca1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2615,7 +2615,7 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
if (IS_ERR(exp))
return PTR_ERR(exp);
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
return err;
}
@@ -3366,7 +3366,7 @@ ctnetlink_create_expect(struct net *net,
goto err_rcu;
}
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
err_rcu:
rcu_read_unlock();
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 976f1dc..0d44a03 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -233,9 +233,9 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)
nf_nat_pptp_exp_gre(exp_orig, exp_reply);
- if (nf_ct_expect_related(exp_orig) != 0)
+ if (nf_ct_expect_related(exp_orig, 0) != 0)
goto out_put_both;
- if (nf_ct_expect_related(exp_reply) != 0)
+ if (nf_ct_expect_related(exp_reply, 0) != 0)
goto out_unexpect_orig;
/* Add GRE keymap entries */
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 8330664..90fade9 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -156,7 +156,7 @@ static int help(struct sk_buff *skb,
nf_ct_dump_tuple(&exp->tuple);
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c30c883..f9e6d1f 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -980,11 +980,15 @@ 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 = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errp == 0 || errp == -EALREADY) {
- int errcp = nf_ct_expect_related(rtcp_exp);
+ int errcp = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errcp == 0 || errcp == -EALREADY)
ret = NF_ACCEPT;
@@ -1299,7 +1303,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
ret = hooks->expect(skb, protoff, dataoff, dptr, datalen,
exp, matchoff, matchlen);
else {
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 6977cb9..2a5931d 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -80,7 +80,7 @@ static int tftp_help(struct sk_buff *skb,
nf_nat_tftp = rcu_dereference(nf_nat_tftp_hook);
if (nf_nat_tftp && ct->status & IPS_NAT_MASK)
ret = nf_nat_tftp(skb, ctinfo, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index 4e59416..63a8d0e 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -52,7 +52,7 @@ static unsigned int help(struct sk_buff *skb,
int res;
exp->tuple.dst.u.tcp.port = htons(port);
- res = nf_ct_expect_related(exp);
+ res = nf_ct_expect_related(exp, 0);
if (res == 0)
break;
else if (res != -EBUSY) {
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 0ea6b1b..620fad9e4 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -94,7 +94,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index d87cbe5..0d83eed 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -57,7 +57,7 @@ static unsigned int help(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 464387b..bce426b 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -417,7 +417,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
int ret;
exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -610,7 +610,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == -EBUSY)
continue;
else if (ret < 0) {
@@ -618,7 +619,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
break;
}
rtcp_exp->tuple.dst.u.udp.port = htons(port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret == -EBUSY) {
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index e633b38..2218890 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -33,7 +33,7 @@ static unsigned int help(struct sk_buff *skb,
= ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
exp->dir = IP_CT_DIR_REPLY;
exp->expectfn = nf_nat_follow_master;
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, exp->master, "cannot add expectation");
return NF_DROP;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5] netfilter: nf_conntrack_sip: fix expectation clash
2019-06-04 8:34 ` [PATCH v5] " xiao ruizhu
@ 2019-06-10 17:45 ` Pablo Neira Ayuso
2019-06-11 5:20 ` [PATCH v6] " xiao ruizhu
0 siblings, 1 reply; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-06-10 17:45 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, alin.nastac, netfilter-devel
Looks good, only one more little change and we go.
On Tue, Jun 04, 2019 at 04:34:23PM +0800, xiao ruizhu wrote:
[...]
> @@ -420,8 +421,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
> }
> h = nf_ct_expect_dst_hash(net, &expect->tuple);
> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
> - if (expect_matches(i, expect)) {
> - if (i->class != expect->class)
> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
> + expect->master) && expect_matches(i, expect)) {
Could you add a function for this? eg.
static bool nf_ct_check_master(struct nf_conntrack_expect *a,
struct nf_conntrack_expect *b)
{
if (flags & NF_CT_EXP_F_CHECK_MASTER)
return true;
return i->master == expect->master &&
expect_matches(i, expect);
}
Was that the intention?
I'm a bit confused with the use of the single statement branch above.
Thanks!
> + if (i->class != expect->class ||
> + i->master != expect->master)
> return -EALREADY;
>
> if (nf_ct_remove_expect(i))
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6] netfilter: nf_conntrack_sip: fix expectation clash
2019-06-10 17:45 ` Pablo Neira Ayuso
@ 2019-06-11 5:20 ` xiao ruizhu
2019-06-17 22:37 ` Pablo Neira Ayuso
0 siblings, 1 reply; 15+ messages in thread
From: xiao ruizhu @ 2019-06-11 5:20 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
> On Tue, Jun 11, 2019 at 01:45AM, Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
> Looks good, only one more little change and we go.
>> On Tue, Jun 04, 2019 at 04:34:23PM +0800, xiao ruizhu wrote:
>> [...]
>> @@ -420,8 +421,10 @@ static inline int __nf_ct_expect_check(struct
>> nf_conntrack_expect *expect)
>> }
>> h = nf_ct_expect_dst_hash(net, &expect->tuple);
>> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
>> - if (expect_matches(i, expect)) {
>> - if (i->class != expect->class)
>> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
>> + expect->master) && expect_matches(i, expect)) {
>
> Could you add a function for this? eg.
>
> static bool nf_ct_check_master(struct nf_conntrack_expect *a,
> struct nf_conntrack_expect *b)
> {
> if (flags & NF_CT_EXP_F_CHECK_MASTER)
> return true;
>
> return i->master == expect->master &&
> expect_matches(i, expect);
> }
> Was that the intention?
> I'm a bit confused with the use of the single statement branch above.
Hi Pablo,
Thanks for your notice.
Sorry, I made a mistake here. I meant to move the checking of master from
expect_matches() to __nf_ct_expect_check(), where we will use the flag
NF_CT_EXP_F_CHECK_MASTER to decide whether masters also need to be checked
or not for matching.
That is, the intention is to change expect_matches() from the original
{
return a->master == b->master &&
nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
}
to
{
return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
}
And in __nf_ct_expect_check(), if the expect is for SIP helper (i.e. with
NF_CT_EXP_F_CHECK_MASTER set), the master will not be checked, only the
items in new expect_matches() will be used for matching check; otherwise,
masters will also be checked. That's the purpose of (flags &
NF_CT_EXP_F_CHECK_MASTER ? true : i->master == expect->master).
However, the modification of expect_matches() was missed to merge into the
patch submitted for review, and causes the confusion. I'm sorry for that.
The modification of expect_matches() is added to patch v6, and please find
it below. Thanks.
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 reuse an existing expect who has the same tuple for a
different conntrack if any.
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@gmail.com>
---
Changes in v6:
- add the modification of expect_matches().
Changes in v5:
- take Pablo's proposal to use a flag in order not to change behavior of
the other existing helpers.
Changes in v4:
- take Pablo's proposal to handle checking in __nf_ct_expect_check().
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
---
include/net/netfilter/nf_conntrack_expect.h | 12 +++++++++---
net/ipv4/netfilter/nf_nat_h323.c | 12 ++++++------
net/netfilter/ipvs/ip_vs_nfct.c | 2 +-
net/netfilter/nf_conntrack_amanda.c | 2 +-
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_conntrack_expect.c | 16 +++++++++-------
net/netfilter/nf_conntrack_ftp.c | 2 +-
net/netfilter/nf_conntrack_h323_main.c | 18 +++++++++---------
net/netfilter/nf_conntrack_irc.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 4 ++--
net/netfilter/nf_conntrack_pptp.c | 4 ++--
net/netfilter/nf_conntrack_sane.c | 2 +-
net/netfilter/nf_conntrack_sip.c | 10 +++++++---
net/netfilter/nf_conntrack_tftp.c | 2 +-
net/netfilter/nf_nat_amanda.c | 2 +-
net/netfilter/nf_nat_ftp.c | 2 +-
net/netfilter/nf_nat_irc.c | 2 +-
net/netfilter/nf_nat_sip.c | 8 +++++---
net/netfilter/nf_nat_tftp.c | 2 +-
19 files changed, 60 insertions(+), 46 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 93ce6b0..6da901e 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -76,6 +76,11 @@ struct nf_conntrack_expect_policy {
#define NF_CT_EXPECT_CLASS_DEFAULT 0
#define NF_CT_EXPECT_MAX_CNT 255
+/* Allow to reuse expectations with the same tuples from different master
+ * conntracks.
+ */
+#define NF_CT_EXP_F_CHECK_MASTER 0x1
+
int nf_conntrack_expect_pernet_init(struct net *net);
void nf_conntrack_expect_pernet_fini(struct net *net);
@@ -122,10 +127,11 @@ void nf_ct_expect_init(struct nf_conntrack_expect *, unsigned int, u_int8_t,
u_int8_t, const __be16 *, const __be16 *);
void nf_ct_expect_put(struct nf_conntrack_expect *exp);
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report);
-static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect)
+ u32 portid, int report, unsigned int flags);
+static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
- return nf_ct_expect_related_report(expect, 0, 0);
+ return nf_ct_expect_related_report(expect, 0, 0, flags);
}
#endif /*_NF_CONNTRACK_EXPECT_H*/
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 7875c98..f06709a 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -222,11 +222,11 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(nated_port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp, 0);
if (ret == 0) {
rtcp_exp->tuple.dst.u.udp.port =
htons(nated_port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp, 0);
if (ret == 0)
break;
else if (ret == -EBUSY) {
@@ -297,7 +297,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -353,7 +353,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -445,7 +445,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -538,7 +538,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index eb8b9c8..45bb5a3 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -247,7 +247,7 @@ void ip_vs_nfct_expect_related(struct sk_buff *skb, struct nf_conn *ct,
IP_VS_DBG_BUF(7, "%s: ct=%p, expect tuple=" FMT_TUPLE "\n",
__func__, ct, ARG_TUPLE(&exp->tuple));
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
}
EXPORT_SYMBOL(ip_vs_nfct_expect_related);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index dbec6fc..5d2f72b 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -163,7 +163,7 @@ static int amanda_help(struct sk_buff *skb,
if (nf_nat_amanda && ct->status & IPS_NAT_MASK)
ret = nf_nat_amanda(skb, ctinfo, protoff,
off - dataoff, len, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 5423b19..1563c86 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -67,7 +67,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
exp->class = NF_CT_EXPECT_CLASS_DEFAULT;
exp->helper = NULL;
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
nf_ct_refresh(ct, skb, timeout * HZ);
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 59c1880..adc26d7 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -252,8 +252,7 @@ static inline int expect_clash(const struct nf_conntrack_expect *a,
static inline int expect_matches(const struct nf_conntrack_expect *a,
const struct nf_conntrack_expect *b)
{
- return a->master == b->master &&
- nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
+ return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
@@ -402,7 +401,8 @@ static void evict_oldest_expect(struct nf_conn *master,
nf_ct_remove_expect(last);
}
-static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
+static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
const struct nf_conntrack_expect_policy *p;
struct nf_conntrack_expect *i;
@@ -420,8 +420,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
h = nf_ct_expect_dst_hash(net, &expect->tuple);
hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
- if (expect_matches(i, expect)) {
- if (i->class != expect->class)
+ if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
+ expect->master) && expect_matches(i, expect)) {
+ if (i->class != expect->class ||
+ i->master != expect->master)
return -EALREADY;
if (nf_ct_remove_expect(i))
@@ -456,12 +458,12 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report)
+ u32 portid, int report, unsigned int flags)
{
int ret;
spin_lock_bh(&nf_conntrack_expect_lock);
- ret = __nf_ct_expect_check(expect);
+ ret = __nf_ct_expect_check(expect, flags);
if (ret < 0)
goto out;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 32aeac1..1578bfa 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -528,7 +528,7 @@ static int help(struct sk_buff *skb,
protoff, matchoff, matchlen, exp);
else {
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 12de403..207024c 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -306,8 +306,8 @@ static int expect_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_rtp_rtcp(skb, ct, ctinfo, protoff, data, dataoff,
taddr, port, rtp_port, rtp_exp, rtcp_exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(rtp_exp) == 0) {
- if (nf_ct_expect_related(rtcp_exp) == 0) {
+ if (nf_ct_expect_related(rtp_exp, 0) == 0) {
+ if (nf_ct_expect_related(rtcp_exp, 0) == 0) {
pr_debug("nf_ct_h323: expect RTP ");
nf_ct_dump_tuple(&rtp_exp->tuple);
pr_debug("nf_ct_h323: expect RTCP ");
@@ -365,7 +365,7 @@ static int expect_t120(struct sk_buff *skb,
ret = nat_t120(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_h323: expect T.120 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -702,7 +702,7 @@ static int expect_h245(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_h245(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect H.245 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -826,7 +826,7 @@ static int expect_callforwarding(struct sk_buff *skb,
protoff, data, dataoff,
taddr, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect Call Forwarding ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1285,7 +1285,7 @@ static int expect_q931(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_q931(skb, ct, ctinfo, protoff, data,
taddr, i, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
@@ -1350,7 +1350,7 @@ static int process_gcf(struct sk_buff *skb, struct nf_conn *ct,
IPPROTO_UDP, NULL, &port);
exp->helper = nf_conntrack_helper_ras;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect RAS ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1562,7 +1562,7 @@ static int process_acf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1616,7 +1616,7 @@ static int process_lcf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 79e5014..92a118f 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -217,7 +217,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
addr_beg_p - ib_ptr,
addr_end_p - addr_beg_p,
exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct,
"cannot add expectation");
ret = NF_DROP;
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7db79c1..56d3ca1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2615,7 +2615,7 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
if (IS_ERR(exp))
return PTR_ERR(exp);
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
return err;
}
@@ -3366,7 +3366,7 @@ ctnetlink_create_expect(struct net *net,
goto err_rcu;
}
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
err_rcu:
rcu_read_unlock();
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 976f1dc..0d44a03 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -233,9 +233,9 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)
nf_nat_pptp_exp_gre(exp_orig, exp_reply);
- if (nf_ct_expect_related(exp_orig) != 0)
+ if (nf_ct_expect_related(exp_orig, 0) != 0)
goto out_put_both;
- if (nf_ct_expect_related(exp_reply) != 0)
+ if (nf_ct_expect_related(exp_reply, 0) != 0)
goto out_unexpect_orig;
/* Add GRE keymap entries */
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 8330664..90fade9 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -156,7 +156,7 @@ static int help(struct sk_buff *skb,
nf_ct_dump_tuple(&exp->tuple);
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c30c883..f9e6d1f 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -980,11 +980,15 @@ 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 = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errp == 0 || errp == -EALREADY) {
- int errcp = nf_ct_expect_related(rtcp_exp);
+ int errcp = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errcp == 0 || errcp == -EALREADY)
ret = NF_ACCEPT;
@@ -1299,7 +1303,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
ret = hooks->expect(skb, protoff, dataoff, dptr, datalen,
exp, matchoff, matchlen);
else {
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 6977cb9..2a5931d 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -80,7 +80,7 @@ static int tftp_help(struct sk_buff *skb,
nf_nat_tftp = rcu_dereference(nf_nat_tftp_hook);
if (nf_nat_tftp && ct->status & IPS_NAT_MASK)
ret = nf_nat_tftp(skb, ctinfo, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index 4e59416..63a8d0e 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -52,7 +52,7 @@ static unsigned int help(struct sk_buff *skb,
int res;
exp->tuple.dst.u.tcp.port = htons(port);
- res = nf_ct_expect_related(exp);
+ res = nf_ct_expect_related(exp, 0);
if (res == 0)
break;
else if (res != -EBUSY) {
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 0ea6b1b..620fad9e4 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -94,7 +94,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index d87cbe5..0d83eed 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -57,7 +57,7 @@ static unsigned int help(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 464387b..bce426b 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -417,7 +417,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
int ret;
exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -610,7 +610,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == -EBUSY)
continue;
else if (ret < 0) {
@@ -618,7 +619,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
break;
}
rtcp_exp->tuple.dst.u.udp.port = htons(port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret == -EBUSY) {
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index e633b38..2218890 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -33,7 +33,7 @@ static unsigned int help(struct sk_buff *skb,
= ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
exp->dir = IP_CT_DIR_REPLY;
exp->expectfn = nf_nat_follow_master;
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, exp->master, "cannot add expectation");
return NF_DROP;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v6] netfilter: nf_conntrack_sip: fix expectation clash
2019-06-11 5:20 ` [PATCH v6] " xiao ruizhu
@ 2019-06-17 22:37 ` Pablo Neira Ayuso
2019-06-18 8:32 ` [PATCH v7] " xiao ruizhu
0 siblings, 1 reply; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-06-17 22:37 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, alin.nastac, netfilter-devel
On Tue, Jun 11, 2019 at 01:20:59PM +0800, xiao ruizhu wrote:
> > On Tue, Jun 11, 2019 at 01:45AM, Pablo Neira Ayuso <pablo@netfilter.org>
> > wrote:
>
> > Looks good, only one more little change and we go.
>
> >> On Tue, Jun 04, 2019 at 04:34:23PM +0800, xiao ruizhu wrote:
> >> [...]
> >> @@ -420,8 +421,10 @@ static inline int __nf_ct_expect_check(struct
> >> nf_conntrack_expect *expect)
> >> }
> >> h = nf_ct_expect_dst_hash(net, &expect->tuple);
> >> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
> >> - if (expect_matches(i, expect)) {
> >> - if (i->class != expect->class)
> >> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
> >> + expect->master) && expect_matches(i, expect)) {
> >
> > Could you add a function for this? eg.
> >
> > static bool nf_ct_check_master(struct nf_conntrack_expect *a,
> > struct nf_conntrack_expect *b)
> > {
> > if (flags & NF_CT_EXP_F_CHECK_MASTER)
> > return true;
> >
> > return i->master == expect->master &&
> > expect_matches(i, expect);
> > }
>
> > Was that the intention?
>
> > I'm a bit confused with the use of the single statement branch above.
>
> Hi Pablo,
>
> Thanks for your notice.
> Sorry, I made a mistake here. I meant to move the checking of master from
> expect_matches() to __nf_ct_expect_check(), where we will use the flag
> NF_CT_EXP_F_CHECK_MASTER to decide whether masters also need to be checked
> or not for matching.
> That is, the intention is to change expect_matches() from the original
> {
> return a->master == b->master &&
> nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
> net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
> nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
> }
> to
> {
> return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
> net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
> nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
> }
> And in __nf_ct_expect_check(), if the expect is for SIP helper (i.e. with
> NF_CT_EXP_F_CHECK_MASTER set), the master will not be checked, only the
> items in new expect_matches() will be used for matching check; otherwise,
> masters will also be checked. That's the purpose of (flags &
> NF_CT_EXP_F_CHECK_MASTER ? true : i->master == expect->master).
[...]
> @@ -420,8 +420,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
> }
> h = nf_ct_expect_dst_hash(net, &expect->tuple);
> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
> - if (expect_matches(i, expect)) {
> - if (i->class != expect->class)
> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
> + expect->master) && expect_matches(i, expect)) {
This part is slightly hard to read. Could you add a function? For
example:
static bool master_matches(...)
{
if (flags & NF_CT_EXP_F_CHECK_MASTER)
return true;
return i->master == expect->master;
}
Then use it:
if (master_matches(i, expect) &&
expect_matches(i, expect)) {
> + if (i->class != expect->class ||
> + i->master != expect->master)
> return -EALREADY;
>
> if (nf_ct_remove_expect(i))
Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v7] netfilter: nf_conntrack_sip: fix expectation clash
2019-06-17 22:37 ` Pablo Neira Ayuso
@ 2019-06-18 8:32 ` xiao ruizhu
2019-07-02 23:51 ` Pablo Neira Ayuso
0 siblings, 1 reply; 15+ messages in thread
From: xiao ruizhu @ 2019-06-18 8:32 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
[...]
> This part is slightly hard to read. Could you add a function? For
> example:
>
> static bool master_matches(...)
> {
> if (flags & NF_CT_EXP_F_CHECK_MASTER)
> return true;
>
> return i->master == expect->master;
> }
>
> Then use it:
>
> if (master_matches(i, expect) &&
> expect_matches(i, expect)) {
> > + if (i->class != expect->class ||
> > + i->master != expect->master)
> > return -EALREADY;
> >
> > if (nf_ct_remove_expect(i))
Hi Pablo,
Thanks for your time and your proposal.
Please find the patch v7 below updated as per your proposal.
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 reuse an existing expect who has the same tuple for a
different conntrack if any.
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@gmail.com>
---
Changes in v7:
- take Pablo's proposal to add a function master_matches().
Changes in v6:
- add the modification of expect_matches().
Changes in v5:
- take Pablo's proposal to use a flag in order not to change behavior of
the other existing helpers.
Changes in v4:
- take Pablo's proposal to handle checking in __nf_ct_expect_check().
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
---
include/net/netfilter/nf_conntrack_expect.h | 12 +++++++++---
net/ipv4/netfilter/nf_nat_h323.c | 12 ++++++------
net/netfilter/ipvs/ip_vs_nfct.c | 2 +-
net/netfilter/nf_conntrack_amanda.c | 2 +-
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_conntrack_expect.c | 26 +++++++++++++++++++-------
net/netfilter/nf_conntrack_ftp.c | 2 +-
net/netfilter/nf_conntrack_h323_main.c | 18 +++++++++---------
net/netfilter/nf_conntrack_irc.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 4 ++--
net/netfilter/nf_conntrack_pptp.c | 4 ++--
net/netfilter/nf_conntrack_sane.c | 2 +-
net/netfilter/nf_conntrack_sip.c | 10 +++++++---
net/netfilter/nf_conntrack_tftp.c | 2 +-
net/netfilter/nf_nat_amanda.c | 2 +-
net/netfilter/nf_nat_ftp.c | 2 +-
net/netfilter/nf_nat_irc.c | 2 +-
net/netfilter/nf_nat_sip.c | 8 +++++---
net/netfilter/nf_nat_tftp.c | 2 +-
19 files changed, 70 insertions(+), 46 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 93ce6b0..6da901e 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -76,6 +76,11 @@ struct nf_conntrack_expect_policy {
#define NF_CT_EXPECT_CLASS_DEFAULT 0
#define NF_CT_EXPECT_MAX_CNT 255
+/* Allow to reuse expectations with the same tuples from different master
+ * conntracks.
+ */
+#define NF_CT_EXP_F_CHECK_MASTER 0x1
+
int nf_conntrack_expect_pernet_init(struct net *net);
void nf_conntrack_expect_pernet_fini(struct net *net);
@@ -122,10 +127,11 @@ void nf_ct_expect_init(struct nf_conntrack_expect *, unsigned int, u_int8_t,
u_int8_t, const __be16 *, const __be16 *);
void nf_ct_expect_put(struct nf_conntrack_expect *exp);
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report);
-static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect)
+ u32 portid, int report, unsigned int flags);
+static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
- return nf_ct_expect_related_report(expect, 0, 0);
+ return nf_ct_expect_related_report(expect, 0, 0, flags);
}
#endif /*_NF_CONNTRACK_EXPECT_H*/
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 7875c98..f06709a 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -222,11 +222,11 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(nated_port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp, 0);
if (ret == 0) {
rtcp_exp->tuple.dst.u.udp.port =
htons(nated_port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp, 0);
if (ret == 0)
break;
else if (ret == -EBUSY) {
@@ -297,7 +297,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -353,7 +353,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -445,7 +445,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -538,7 +538,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index eb8b9c8..45bb5a3 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -247,7 +247,7 @@ void ip_vs_nfct_expect_related(struct sk_buff *skb, struct nf_conn *ct,
IP_VS_DBG_BUF(7, "%s: ct=%p, expect tuple=" FMT_TUPLE "\n",
__func__, ct, ARG_TUPLE(&exp->tuple));
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
}
EXPORT_SYMBOL(ip_vs_nfct_expect_related);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index dbec6fc..5d2f72b 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -163,7 +163,7 @@ static int amanda_help(struct sk_buff *skb,
if (nf_nat_amanda && ct->status & IPS_NAT_MASK)
ret = nf_nat_amanda(skb, ctinfo, protoff,
off - dataoff, len, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 5423b19..1563c86 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -67,7 +67,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
exp->class = NF_CT_EXPECT_CLASS_DEFAULT;
exp->helper = NULL;
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
nf_ct_refresh(ct, skb, timeout * HZ);
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 59c1880..7df6228 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -252,13 +252,22 @@ static inline int expect_clash(const struct nf_conntrack_expect *a,
static inline int expect_matches(const struct nf_conntrack_expect *a,
const struct nf_conntrack_expect *b)
{
- return a->master == b->master &&
- nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
+ return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
}
+static bool master_matches(const struct nf_conntrack_expect *a,
+ const struct nf_conntrack_expect *b,
+ unsigned int flags)
+{
+ if (flags & NF_CT_EXP_F_CHECK_MASTER)
+ return true;
+
+ return a->master == b->master;
+}
+
/* Generally a bad idea to call this: could have matched already. */
void nf_ct_unexpect_related(struct nf_conntrack_expect *exp)
{
@@ -402,7 +411,8 @@ static void evict_oldest_expect(struct nf_conn *master,
nf_ct_remove_expect(last);
}
-static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
+static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
const struct nf_conntrack_expect_policy *p;
struct nf_conntrack_expect *i;
@@ -420,8 +430,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
h = nf_ct_expect_dst_hash(net, &expect->tuple);
hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
- if (expect_matches(i, expect)) {
- if (i->class != expect->class)
+ if (master_matches(i, expect, flags) &&
+ expect_matches(i, expect)) {
+ if (i->class != expect->class ||
+ i->master != expect->master)
return -EALREADY;
if (nf_ct_remove_expect(i))
@@ -456,12 +468,12 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report)
+ u32 portid, int report, unsigned int flags)
{
int ret;
spin_lock_bh(&nf_conntrack_expect_lock);
- ret = __nf_ct_expect_check(expect);
+ ret = __nf_ct_expect_check(expect, flags);
if (ret < 0)
goto out;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 32aeac1..1578bfa 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -528,7 +528,7 @@ static int help(struct sk_buff *skb,
protoff, matchoff, matchlen, exp);
else {
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 12de403..207024c 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -306,8 +306,8 @@ static int expect_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_rtp_rtcp(skb, ct, ctinfo, protoff, data, dataoff,
taddr, port, rtp_port, rtp_exp, rtcp_exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(rtp_exp) == 0) {
- if (nf_ct_expect_related(rtcp_exp) == 0) {
+ if (nf_ct_expect_related(rtp_exp, 0) == 0) {
+ if (nf_ct_expect_related(rtcp_exp, 0) == 0) {
pr_debug("nf_ct_h323: expect RTP ");
nf_ct_dump_tuple(&rtp_exp->tuple);
pr_debug("nf_ct_h323: expect RTCP ");
@@ -365,7 +365,7 @@ static int expect_t120(struct sk_buff *skb,
ret = nat_t120(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_h323: expect T.120 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -702,7 +702,7 @@ static int expect_h245(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_h245(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect H.245 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -826,7 +826,7 @@ static int expect_callforwarding(struct sk_buff *skb,
protoff, data, dataoff,
taddr, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect Call Forwarding ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1285,7 +1285,7 @@ static int expect_q931(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_q931(skb, ct, ctinfo, protoff, data,
taddr, i, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
@@ -1350,7 +1350,7 @@ static int process_gcf(struct sk_buff *skb, struct nf_conn *ct,
IPPROTO_UDP, NULL, &port);
exp->helper = nf_conntrack_helper_ras;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect RAS ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1562,7 +1562,7 @@ static int process_acf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1616,7 +1616,7 @@ static int process_lcf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 79e5014..92a118f 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -217,7 +217,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
addr_beg_p - ib_ptr,
addr_end_p - addr_beg_p,
exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct,
"cannot add expectation");
ret = NF_DROP;
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7db79c1..56d3ca1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2615,7 +2615,7 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
if (IS_ERR(exp))
return PTR_ERR(exp);
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
return err;
}
@@ -3366,7 +3366,7 @@ ctnetlink_create_expect(struct net *net,
goto err_rcu;
}
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
err_rcu:
rcu_read_unlock();
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 976f1dc..0d44a03 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -233,9 +233,9 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)
nf_nat_pptp_exp_gre(exp_orig, exp_reply);
- if (nf_ct_expect_related(exp_orig) != 0)
+ if (nf_ct_expect_related(exp_orig, 0) != 0)
goto out_put_both;
- if (nf_ct_expect_related(exp_reply) != 0)
+ if (nf_ct_expect_related(exp_reply, 0) != 0)
goto out_unexpect_orig;
/* Add GRE keymap entries */
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 8330664..90fade9 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -156,7 +156,7 @@ static int help(struct sk_buff *skb,
nf_ct_dump_tuple(&exp->tuple);
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c30c883..f9e6d1f 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -980,11 +980,15 @@ 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 = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errp == 0 || errp == -EALREADY) {
- int errcp = nf_ct_expect_related(rtcp_exp);
+ int errcp = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (errcp == 0 || errcp == -EALREADY)
ret = NF_ACCEPT;
@@ -1299,7 +1303,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
ret = hooks->expect(skb, protoff, dataoff, dptr, datalen,
exp, matchoff, matchlen);
else {
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 6977cb9..2a5931d 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -80,7 +80,7 @@ static int tftp_help(struct sk_buff *skb,
nf_nat_tftp = rcu_dereference(nf_nat_tftp_hook);
if (nf_nat_tftp && ct->status & IPS_NAT_MASK)
ret = nf_nat_tftp(skb, ctinfo, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index 4e59416..63a8d0e 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -52,7 +52,7 @@ static unsigned int help(struct sk_buff *skb,
int res;
exp->tuple.dst.u.tcp.port = htons(port);
- res = nf_ct_expect_related(exp);
+ res = nf_ct_expect_related(exp, 0);
if (res == 0)
break;
else if (res != -EBUSY) {
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 0ea6b1b..620fad9e4 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -94,7 +94,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index d87cbe5..0d83eed 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -57,7 +57,7 @@ static unsigned int help(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 464387b..bce426b 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -417,7 +417,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
int ret;
exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -610,7 +610,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == -EBUSY)
continue;
else if (ret < 0) {
@@ -618,7 +619,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
break;
}
rtcp_exp->tuple.dst.u.udp.port = htons(port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_CHECK_MASTER);
if (ret == 0)
break;
else if (ret == -EBUSY) {
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index e633b38..2218890 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -33,7 +33,7 @@ static unsigned int help(struct sk_buff *skb,
= ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
exp->dir = IP_CT_DIR_REPLY;
exp->expectfn = nf_nat_follow_master;
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, exp->master, "cannot add expectation");
return NF_DROP;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v7] netfilter: nf_conntrack_sip: fix expectation clash
2019-06-18 8:32 ` [PATCH v7] " xiao ruizhu
@ 2019-07-02 23:51 ` Pablo Neira Ayuso
2019-07-04 3:31 ` [PATCH v8] " xiao ruizhu
0 siblings, 1 reply; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-07-02 23:51 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, alin.nastac, netfilter-devel
This patch looks nice now.
One more change and we go:
On Tue, Jun 18, 2019 at 04:32:50PM +0800, xiao ruizhu wrote:
> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> index 59c1880..7df6228 100644
> --- a/net/netfilter/nf_conntrack_expect.c
> +++ b/net/netfilter/nf_conntrack_expect.c
> @@ -252,13 +252,22 @@ static inline int expect_clash(const struct nf_conntrack_expect *a,
> static inline int expect_matches(const struct nf_conntrack_expect *a,
> const struct nf_conntrack_expect *b)
> {
> - return a->master == b->master &&
> - nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> + return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
> net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
> nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
> }
>
> +static bool master_matches(const struct nf_conntrack_expect *a,
> + const struct nf_conntrack_expect *b,
> + unsigned int flags)
> +{
> + if (flags & NF_CT_EXP_F_CHECK_MASTER)
rename this to NF_CT_EXP_F_SKIP_MASTER.
Since semantics here is to skip the master check, rather than checking
for it.
> + return true;
> +
> + return a->master == b->master;
> +}
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v8] netfilter: nf_conntrack_sip: fix expectation clash
2019-07-02 23:51 ` Pablo Neira Ayuso
@ 2019-07-04 3:31 ` xiao ruizhu
2019-07-16 11:17 ` Pablo Neira Ayuso
0 siblings, 1 reply; 15+ messages in thread
From: xiao ruizhu @ 2019-07-04 3:31 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, alin.nastac, netfilter-devel; +Cc: xiao ruizhu
Hi Pablo,
NF_CT_EXP_F_CHECK_MASTER was renamed to NF_CT_EXP_F_SKIP_MASTER.
Please find the patch updated below. Thanks.
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 reuse an existing expect who has the same tuple for a
different conntrack if any.
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@gmail.com>
---
Changes in v8:
- change NF_CT_EXP_F_CHECK_MASTER to NF_CT_EXP_F_SKIP_MASTER for better
semantics match.
Changes in v7:
- take Pablo's proposal to add a function master_matches().
Changes in v6:
- add the modification of expect_matches().
Changes in v5:
- take Pablo's proposal to use a flag in order not to change behavior of
the other existing helpers.
Changes in v4:
- take Pablo's proposal to handle checking in __nf_ct_expect_check().
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
---
include/net/netfilter/nf_conntrack_expect.h | 12 +++++++++---
net/ipv4/netfilter/nf_nat_h323.c | 12 ++++++------
net/netfilter/ipvs/ip_vs_nfct.c | 2 +-
net/netfilter/nf_conntrack_amanda.c | 2 +-
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_conntrack_expect.c | 26 +++++++++++++++++++-------
net/netfilter/nf_conntrack_ftp.c | 2 +-
net/netfilter/nf_conntrack_h323_main.c | 18 +++++++++---------
net/netfilter/nf_conntrack_irc.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 4 ++--
net/netfilter/nf_conntrack_pptp.c | 4 ++--
net/netfilter/nf_conntrack_sane.c | 2 +-
net/netfilter/nf_conntrack_sip.c | 10 +++++++---
net/netfilter/nf_conntrack_tftp.c | 2 +-
net/netfilter/nf_nat_amanda.c | 2 +-
net/netfilter/nf_nat_ftp.c | 2 +-
net/netfilter/nf_nat_irc.c | 2 +-
net/netfilter/nf_nat_sip.c | 8 +++++---
net/netfilter/nf_nat_tftp.c | 2 +-
19 files changed, 70 insertions(+), 46 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 93ce6b0..573429b 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -76,6 +76,11 @@ struct nf_conntrack_expect_policy {
#define NF_CT_EXPECT_CLASS_DEFAULT 0
#define NF_CT_EXPECT_MAX_CNT 255
+/* Allow to reuse expectations with the same tuples from different master
+ * conntracks.
+ */
+#define NF_CT_EXP_F_SKIP_MASTER 0x1
+
int nf_conntrack_expect_pernet_init(struct net *net);
void nf_conntrack_expect_pernet_fini(struct net *net);
@@ -122,10 +127,11 @@ void nf_ct_expect_init(struct nf_conntrack_expect *, unsigned int, u_int8_t,
u_int8_t, const __be16 *, const __be16 *);
void nf_ct_expect_put(struct nf_conntrack_expect *exp);
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report);
-static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect)
+ u32 portid, int report, unsigned int flags);
+static inline int nf_ct_expect_related(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
- return nf_ct_expect_related_report(expect, 0, 0);
+ return nf_ct_expect_related_report(expect, 0, 0, flags);
}
#endif /*_NF_CONNTRACK_EXPECT_H*/
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 7875c98..f06709a 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -222,11 +222,11 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(nated_port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp, 0);
if (ret == 0) {
rtcp_exp->tuple.dst.u.udp.port =
htons(nated_port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp, 0);
if (ret == 0)
break;
else if (ret == -EBUSY) {
@@ -297,7 +297,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -353,7 +353,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -445,7 +445,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -538,7 +538,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
int ret;
exp->tuple.dst.u.tcp.port = htons(nated_port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index eb8b9c8..45bb5a3 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -247,7 +247,7 @@ void ip_vs_nfct_expect_related(struct sk_buff *skb, struct nf_conn *ct,
IP_VS_DBG_BUF(7, "%s: ct=%p, expect tuple=" FMT_TUPLE "\n",
__func__, ct, ARG_TUPLE(&exp->tuple));
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
}
EXPORT_SYMBOL(ip_vs_nfct_expect_related);
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index dbec6fc..5d2f72b 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -163,7 +163,7 @@ static int amanda_help(struct sk_buff *skb,
if (nf_nat_amanda && ct->status & IPS_NAT_MASK)
ret = nf_nat_amanda(skb, ctinfo, protoff,
off - dataoff, len, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 5423b19..1563c86 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -67,7 +67,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
exp->class = NF_CT_EXPECT_CLASS_DEFAULT;
exp->helper = NULL;
- nf_ct_expect_related(exp);
+ nf_ct_expect_related(exp, 0);
nf_ct_expect_put(exp);
nf_ct_refresh(ct, skb, timeout * HZ);
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 59c1880..2cb935b 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -252,13 +252,22 @@ static inline int expect_clash(const struct nf_conntrack_expect *a,
static inline int expect_matches(const struct nf_conntrack_expect *a,
const struct nf_conntrack_expect *b)
{
- return a->master == b->master &&
- nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
+ return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
}
+static bool master_matches(const struct nf_conntrack_expect *a,
+ const struct nf_conntrack_expect *b,
+ unsigned int flags)
+{
+ if (flags & NF_CT_EXP_F_SKIP_MASTER)
+ return true;
+
+ return a->master == b->master;
+}
+
/* Generally a bad idea to call this: could have matched already. */
void nf_ct_unexpect_related(struct nf_conntrack_expect *exp)
{
@@ -402,7 +411,8 @@ static void evict_oldest_expect(struct nf_conn *master,
nf_ct_remove_expect(last);
}
-static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
+static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
+ unsigned int flags)
{
const struct nf_conntrack_expect_policy *p;
struct nf_conntrack_expect *i;
@@ -420,8 +430,10 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
h = nf_ct_expect_dst_hash(net, &expect->tuple);
hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
- if (expect_matches(i, expect)) {
- if (i->class != expect->class)
+ if (master_matches(i, expect, flags) &&
+ expect_matches(i, expect)) {
+ if (i->class != expect->class ||
+ i->master != expect->master)
return -EALREADY;
if (nf_ct_remove_expect(i))
@@ -456,12 +468,12 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
}
int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
- u32 portid, int report)
+ u32 portid, int report, unsigned int flags)
{
int ret;
spin_lock_bh(&nf_conntrack_expect_lock);
- ret = __nf_ct_expect_check(expect);
+ ret = __nf_ct_expect_check(expect, flags);
if (ret < 0)
goto out;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 32aeac1..1578bfa 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -528,7 +528,7 @@ static int help(struct sk_buff *skb,
protoff, matchoff, matchlen, exp);
else {
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 12de403..207024c 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -306,8 +306,8 @@ static int expect_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_rtp_rtcp(skb, ct, ctinfo, protoff, data, dataoff,
taddr, port, rtp_port, rtp_exp, rtcp_exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(rtp_exp) == 0) {
- if (nf_ct_expect_related(rtcp_exp) == 0) {
+ if (nf_ct_expect_related(rtp_exp, 0) == 0) {
+ if (nf_ct_expect_related(rtcp_exp, 0) == 0) {
pr_debug("nf_ct_h323: expect RTP ");
nf_ct_dump_tuple(&rtp_exp->tuple);
pr_debug("nf_ct_h323: expect RTCP ");
@@ -365,7 +365,7 @@ static int expect_t120(struct sk_buff *skb,
ret = nat_t120(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_h323: expect T.120 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -702,7 +702,7 @@ static int expect_h245(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_h245(skb, ct, ctinfo, protoff, data, dataoff, taddr,
port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect H.245 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -826,7 +826,7 @@ static int expect_callforwarding(struct sk_buff *skb,
protoff, data, dataoff,
taddr, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_q931: expect Call Forwarding ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1285,7 +1285,7 @@ static int expect_q931(struct sk_buff *skb, struct nf_conn *ct,
ret = nat_q931(skb, ct, ctinfo, protoff, data,
taddr, i, port, exp);
} else { /* Conntrack only */
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
@@ -1350,7 +1350,7 @@ static int process_gcf(struct sk_buff *skb, struct nf_conn *ct,
IPPROTO_UDP, NULL, &port);
exp->helper = nf_conntrack_helper_ras;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect RAS ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1562,7 +1562,7 @@ static int process_acf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
@@ -1616,7 +1616,7 @@ static int process_lcf(struct sk_buff *skb, struct nf_conn *ct,
exp->flags = NF_CT_EXPECT_PERMANENT;
exp->helper = nf_conntrack_helper_q931;
- if (nf_ct_expect_related(exp) == 0) {
+ if (nf_ct_expect_related(exp, 0) == 0) {
pr_debug("nf_ct_ras: expect Q.931 ");
nf_ct_dump_tuple(&exp->tuple);
} else
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 79e5014..92a118f 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -217,7 +217,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
addr_beg_p - ib_ptr,
addr_end_p - addr_beg_p,
exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct,
"cannot add expectation");
ret = NF_DROP;
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7db79c1..56d3ca1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2615,7 +2615,7 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
if (IS_ERR(exp))
return PTR_ERR(exp);
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
return err;
}
@@ -3366,7 +3366,7 @@ ctnetlink_create_expect(struct net *net,
goto err_rcu;
}
- err = nf_ct_expect_related_report(exp, portid, report);
+ err = nf_ct_expect_related_report(exp, portid, report, 0);
nf_ct_expect_put(exp);
err_rcu:
rcu_read_unlock();
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 976f1dc..0d44a03 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -233,9 +233,9 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)
nf_nat_pptp_exp_gre(exp_orig, exp_reply);
- if (nf_ct_expect_related(exp_orig) != 0)
+ if (nf_ct_expect_related(exp_orig, 0) != 0)
goto out_put_both;
- if (nf_ct_expect_related(exp_reply) != 0)
+ if (nf_ct_expect_related(exp_reply, 0) != 0)
goto out_unexpect_orig;
/* Add GRE keymap entries */
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 8330664..90fade9 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -156,7 +156,7 @@ static int help(struct sk_buff *skb,
nf_ct_dump_tuple(&exp->tuple);
/* Can't expect this? Best to drop packet now. */
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c30c883..04efe1e 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -980,11 +980,15 @@ 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 = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_SKIP_MASTER);
if (errp == 0 || errp == -EALREADY) {
- int errcp = nf_ct_expect_related(rtcp_exp);
+ int errcp = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_SKIP_MASTER);
if (errcp == 0 || errcp == -EALREADY)
ret = NF_ACCEPT;
@@ -1299,7 +1303,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
ret = hooks->expect(skb, protoff, dataoff, dptr, datalen,
exp, matchoff, matchlen);
else {
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
} else
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index 6977cb9..2a5931d 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -80,7 +80,7 @@ static int tftp_help(struct sk_buff *skb,
nf_nat_tftp = rcu_dereference(nf_nat_tftp_hook);
if (nf_nat_tftp && ct->status & IPS_NAT_MASK)
ret = nf_nat_tftp(skb, ctinfo, exp);
- else if (nf_ct_expect_related(exp) != 0) {
+ else if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, ct, "cannot add expectation");
ret = NF_DROP;
}
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index 4e59416..63a8d0e 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -52,7 +52,7 @@ static unsigned int help(struct sk_buff *skb,
int res;
exp->tuple.dst.u.tcp.port = htons(port);
- res = nf_ct_expect_related(exp);
+ res = nf_ct_expect_related(exp, 0);
if (res == 0)
break;
else if (res != -EBUSY) {
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index 0ea6b1b..620fad9e4 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -94,7 +94,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index d87cbe5..0d83eed 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -57,7 +57,7 @@ static unsigned int help(struct sk_buff *skb,
int ret;
exp->tuple.dst.u.tcp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, 0);
if (ret == 0)
break;
else if (ret != -EBUSY) {
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 464387b..7381bcc 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -417,7 +417,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
int ret;
exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(exp);
+ ret = nf_ct_expect_related(exp, NF_CT_EXP_F_SKIP_MASTER);
if (ret == 0)
break;
else if (ret != -EBUSY) {
@@ -610,7 +610,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
int ret;
rtp_exp->tuple.dst.u.udp.port = htons(port);
- ret = nf_ct_expect_related(rtp_exp);
+ ret = nf_ct_expect_related(rtp_exp,
+ NF_CT_EXP_F_SKIP_MASTER);
if (ret == -EBUSY)
continue;
else if (ret < 0) {
@@ -618,7 +619,8 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
break;
}
rtcp_exp->tuple.dst.u.udp.port = htons(port + 1);
- ret = nf_ct_expect_related(rtcp_exp);
+ ret = nf_ct_expect_related(rtcp_exp,
+ NF_CT_EXP_F_SKIP_MASTER);
if (ret == 0)
break;
else if (ret == -EBUSY) {
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index e633b38..2218890 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -33,7 +33,7 @@ static unsigned int help(struct sk_buff *skb,
= ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
exp->dir = IP_CT_DIR_REPLY;
exp->expectfn = nf_nat_follow_master;
- if (nf_ct_expect_related(exp) != 0) {
+ if (nf_ct_expect_related(exp, 0) != 0) {
nf_ct_helper_log(skb, exp->master, "cannot add expectation");
return NF_DROP;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v8] netfilter: nf_conntrack_sip: fix expectation clash
2019-07-04 3:31 ` [PATCH v8] " xiao ruizhu
@ 2019-07-16 11:17 ` Pablo Neira Ayuso
0 siblings, 0 replies; 15+ messages in thread
From: Pablo Neira Ayuso @ 2019-07-16 11:17 UTC (permalink / raw)
To: xiao ruizhu; +Cc: kadlec, fw, davem, alin.nastac, netfilter-devel
On Thu, Jul 04, 2019 at 11:31:13AM +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 reuse an existing expect who has the same tuple for a
> different conntrack if any.
>
> 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.
Applied to nf.git, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-07-16 11:17 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-14 3:28 [PATCH v2] netfilter: nf_conntrack_sip: fix rtcp expectation clash xiao ruizhu
2019-03-14 8:21 ` Alin Năstac
2019-03-21 10:56 ` Pablo Neira Ayuso
2019-04-15 8:33 ` [PATCH v3] netfilter: nf_conntrack_sip: fix " xiao ruizhu
2019-05-13 11:26 ` Pablo Neira Ayuso
[not found] ` <CAEorUYZe2mtLupMDkAOvMXZoH_NcUOKLR=K4atLC5dddHOs-MQ@mail.gmail.com>
2019-05-14 10:18 ` Pablo Neira Ayuso
2019-05-15 7:45 ` [PATCH v4] " xiao ruizhu
2019-06-04 8:34 ` [PATCH v5] " xiao ruizhu
2019-06-10 17:45 ` Pablo Neira Ayuso
2019-06-11 5:20 ` [PATCH v6] " xiao ruizhu
2019-06-17 22:37 ` Pablo Neira Ayuso
2019-06-18 8:32 ` [PATCH v7] " xiao ruizhu
2019-07-02 23:51 ` Pablo Neira Ayuso
2019-07-04 3:31 ` [PATCH v8] " xiao ruizhu
2019-07-16 11:17 ` Pablo Neira Ayuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.