* [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
@ 2019-03-13 8:33 Xin Long
2019-03-13 11:33 ` Neil Horman
2019-03-18 15:22 ` Pablo Neira Ayuso
0 siblings, 2 replies; 8+ messages in thread
From: Xin Long @ 2019-03-13 8:33 UTC (permalink / raw)
To: network dev, netfilter-devel
Cc: Marcelo Ricardo Leitner, Neil Horman, pablo, fw
Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
skb->transport_header has been always set before entering INET
netfilter. This patch is to set skb->transport_header for bridge
before entering INET netfilter by bridge-nf-call-iptables.
It also fixes an issue that sctp_error() couldn't compute a right
csum due to unset skb->transport_header.
Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
Reported-by: Li Shuang <shuali@redhat.com>
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/bridge/br_netfilter_hooks.c | 1 +
net/bridge/br_netfilter_ipv6.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index c93c35b..4d09a33 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
skb->protocol = htons(ETH_P_IP);
+ skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
skb->dev, NULL,
diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
index 564710f..e88d664 100644
--- a/net/bridge/br_netfilter_ipv6.c
+++ b/net/bridge/br_netfilter_ipv6.c
@@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
skb->protocol = htons(ETH_P_IPV6);
+ skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
+
NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
skb->dev, NULL,
br_nf_pre_routing_finish_ipv6);
--
2.1.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-13 8:33 [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING Xin Long
@ 2019-03-13 11:33 ` Neil Horman
2019-03-13 11:59 ` Florian Westphal
2019-03-18 15:22 ` Pablo Neira Ayuso
1 sibling, 1 reply; 8+ messages in thread
From: Neil Horman @ 2019-03-13 11:33 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, netfilter-devel, Marcelo Ricardo Leitner, pablo, fw
On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> skb->transport_header has been always set before entering INET
> netfilter. This patch is to set skb->transport_header for bridge
> before entering INET netfilter by bridge-nf-call-iptables.
>
> It also fixes an issue that sctp_error() couldn't compute a right
> csum due to unset skb->transport_header.
>
> Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> Reported-by: Li Shuang <shuali@redhat.com>
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/bridge/br_netfilter_hooks.c | 1 +
> net/bridge/br_netfilter_ipv6.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index c93c35b..4d09a33 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
>
> skb->protocol = htons(ETH_P_IP);
> + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
>
> NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> skb->dev, NULL,
> diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> index 564710f..e88d664 100644
> --- a/net/bridge/br_netfilter_ipv6.c
> +++ b/net/bridge/br_netfilter_ipv6.c
> @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
>
> skb->protocol = htons(ETH_P_IPV6);
> + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> +
> NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> skb->dev, NULL,
> br_nf_pre_routing_finish_ipv6);
> --
> 2.1.0
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-13 11:33 ` Neil Horman
@ 2019-03-13 11:59 ` Florian Westphal
2019-03-14 14:19 ` Pablo Neira Ayuso
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2019-03-13 11:59 UTC (permalink / raw)
To: Neil Horman
Cc: Xin Long, network dev, netfilter-devel, Marcelo Ricardo Leitner,
pablo, fw
Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > skb->transport_header has been always set before entering INET
> > netfilter. This patch is to set skb->transport_header for bridge
> > before entering INET netfilter by bridge-nf-call-iptables.
> >
> > It also fixes an issue that sctp_error() couldn't compute a right
> > csum due to unset skb->transport_header.
> >
> > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > Reported-by: Li Shuang <shuali@redhat.com>
> > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> > net/bridge/br_netfilter_hooks.c | 1 +
> > net/bridge/br_netfilter_ipv6.c | 2 ++
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > index c93c35b..4d09a33 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> > nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> >
> > skb->protocol = htons(ETH_P_IP);
> > + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> >
> > NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > skb->dev, NULL,
> > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > index 564710f..e88d664 100644
> > --- a/net/bridge/br_netfilter_ipv6.c
> > +++ b/net/bridge/br_netfilter_ipv6.c
> > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> > nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> >
> > skb->protocol = htons(ETH_P_IPV6);
> > + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > +
> > NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > skb->dev, NULL,
> > br_nf_pre_routing_finish_ipv6);
> > --
> > 2.1.0
> >
> >
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Florian Westphal <fw@strlen.de>
... because this fixes discrepancy of normal stack vs. bridge path.
However, I think we still need a fix for the ipv6 case as
there is no guarantee ipv6 nexthdr will be the sctp one.
We already pass the sctp offset as argument to sctp_compute_cksum(),
why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
Xin Long originally suggested?
IOW, whats the problem with
https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?
(In theory conntrack can also inspect transport header inside icmp error
messages like pkttoobig and so on,
although this won't be the case here as we can't validate csum anyway
if the packet isn't complete).
Just pointing out that we can't rely on skb transport header being
"correct" in all cases from netfilter point of view.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-13 11:59 ` Florian Westphal
@ 2019-03-14 14:19 ` Pablo Neira Ayuso
2019-03-14 14:41 ` Xin Long
0 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2019-03-14 14:19 UTC (permalink / raw)
To: Florian Westphal
Cc: Neil Horman, Xin Long, network dev, netfilter-devel,
Marcelo Ricardo Leitner
On Wed, Mar 13, 2019 at 12:59:48PM +0100, Florian Westphal wrote:
> Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > > skb->transport_header has been always set before entering INET
> > > netfilter. This patch is to set skb->transport_header for bridge
> > > before entering INET netfilter by bridge-nf-call-iptables.
> > >
> > > It also fixes an issue that sctp_error() couldn't compute a right
> > > csum due to unset skb->transport_header.
> > >
> > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > Reported-by: Li Shuang <shuali@redhat.com>
> > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > > net/bridge/br_netfilter_hooks.c | 1 +
> > > net/bridge/br_netfilter_ipv6.c | 2 ++
> > > 2 files changed, 3 insertions(+)
> > >
> > > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > > index c93c35b..4d09a33 100644
> > > --- a/net/bridge/br_netfilter_hooks.c
> > > +++ b/net/bridge/br_netfilter_hooks.c
> > > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> > > nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> > >
> > > skb->protocol = htons(ETH_P_IP);
> > > + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> > >
> > > NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > skb->dev, NULL,
> > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > > index 564710f..e88d664 100644
> > > --- a/net/bridge/br_netfilter_ipv6.c
> > > +++ b/net/bridge/br_netfilter_ipv6.c
> > > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> > > nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> > >
> > > skb->protocol = htons(ETH_P_IPV6);
> > > + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > > +
> > > NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > skb->dev, NULL,
> > > br_nf_pre_routing_finish_ipv6);
> > > --
> > > 2.1.0
> > >
> > >
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
>
> Acked-by: Florian Westphal <fw@strlen.de>
>
> ... because this fixes discrepancy of normal stack vs. bridge path.
>
> However, I think we still need a fix for the ipv6 case as
> there is no guarantee ipv6 nexthdr will be the sctp one.
>
> We already pass the sctp offset as argument to sctp_compute_cksum(),
> why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
> Xin Long originally suggested?
>
> IOW, whats the problem with
> https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?
That patch looks fine too.
> (In theory conntrack can also inspect transport header inside icmp error
> messages like pkttoobig and so on,
> although this won't be the case here as we can't validate csum anyway
> if the packet isn't complete).
>
> Just pointing out that we can't rely on skb transport header being
> "correct" in all cases from netfilter point of view.
Indeed.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-14 14:19 ` Pablo Neira Ayuso
@ 2019-03-14 14:41 ` Xin Long
2019-03-15 11:09 ` Neil Horman
0 siblings, 1 reply; 8+ messages in thread
From: Xin Long @ 2019-03-14 14:41 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Florian Westphal, Neil Horman, network dev, netfilter-devel,
Marcelo Ricardo Leitner
On Thu, Mar 14, 2019 at 10:19 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Mar 13, 2019 at 12:59:48PM +0100, Florian Westphal wrote:
> > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > > > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > > > skb->transport_header has been always set before entering INET
> > > > netfilter. This patch is to set skb->transport_header for bridge
> > > > before entering INET netfilter by bridge-nf-call-iptables.
> > > >
> > > > It also fixes an issue that sctp_error() couldn't compute a right
> > > > csum due to unset skb->transport_header.
> > > >
> > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > Reported-by: Li Shuang <shuali@redhat.com>
> > > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > > net/bridge/br_netfilter_hooks.c | 1 +
> > > > net/bridge/br_netfilter_ipv6.c | 2 ++
> > > > 2 files changed, 3 insertions(+)
> > > >
> > > > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > > > index c93c35b..4d09a33 100644
> > > > --- a/net/bridge/br_netfilter_hooks.c
> > > > +++ b/net/bridge/br_netfilter_hooks.c
> > > > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> > > > nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> > > >
> > > > skb->protocol = htons(ETH_P_IP);
> > > > + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> > > >
> > > > NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > skb->dev, NULL,
> > > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > > > index 564710f..e88d664 100644
> > > > --- a/net/bridge/br_netfilter_ipv6.c
> > > > +++ b/net/bridge/br_netfilter_ipv6.c
> > > > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> > > > nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> > > >
> > > > skb->protocol = htons(ETH_P_IPV6);
> > > > + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > > > +
> > > > NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > skb->dev, NULL,
> > > > br_nf_pre_routing_finish_ipv6);
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> > Acked-by: Florian Westphal <fw@strlen.de>
> >
> > ... because this fixes discrepancy of normal stack vs. bridge path.
> >
> > However, I think we still need a fix for the ipv6 case as
> > there is no guarantee ipv6 nexthdr will be the sctp one.
> >
> > We already pass the sctp offset as argument to sctp_compute_cksum(),
> > why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
> > Xin Long originally suggested?
> >
> > IOW, whats the problem with
> > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?
>
> That patch looks fine too.
>
> > (In theory conntrack can also inspect transport header inside icmp error
> > messages like pkttoobig and so on,
> > although this won't be the case here as we can't validate csum anyway
> > if the packet isn't complete).
> >
> > Just pointing out that we can't rely on skb transport header being
> > "correct" in all cases from netfilter point of view.
>
> Indeed.
Hi Neil,
It seems we should bring that patch back, otherwize we will have to add
a special sctp csum computing function for Netfilter?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-14 14:41 ` Xin Long
@ 2019-03-15 11:09 ` Neil Horman
2019-03-15 13:39 ` Xin Long
0 siblings, 1 reply; 8+ messages in thread
From: Neil Horman @ 2019-03-15 11:09 UTC (permalink / raw)
To: Xin Long
Cc: Pablo Neira Ayuso, Florian Westphal, network dev,
netfilter-devel, Marcelo Ricardo Leitner
On Thu, Mar 14, 2019 at 10:41:41PM +0800, Xin Long wrote:
> On Thu, Mar 14, 2019 at 10:19 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Wed, Mar 13, 2019 at 12:59:48PM +0100, Florian Westphal wrote:
> > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > > > > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > > > > skb->transport_header has been always set before entering INET
> > > > > netfilter. This patch is to set skb->transport_header for bridge
> > > > > before entering INET netfilter by bridge-nf-call-iptables.
> > > > >
> > > > > It also fixes an issue that sctp_error() couldn't compute a right
> > > > > csum due to unset skb->transport_header.
> > > > >
> > > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > > Reported-by: Li Shuang <shuali@redhat.com>
> > > > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > > net/bridge/br_netfilter_hooks.c | 1 +
> > > > > net/bridge/br_netfilter_ipv6.c | 2 ++
> > > > > 2 files changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > > > > index c93c35b..4d09a33 100644
> > > > > --- a/net/bridge/br_netfilter_hooks.c
> > > > > +++ b/net/bridge/br_netfilter_hooks.c
> > > > > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> > > > > nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> > > > >
> > > > > skb->protocol = htons(ETH_P_IP);
> > > > > + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> > > > >
> > > > > NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > > skb->dev, NULL,
> > > > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > > > > index 564710f..e88d664 100644
> > > > > --- a/net/bridge/br_netfilter_ipv6.c
> > > > > +++ b/net/bridge/br_netfilter_ipv6.c
> > > > > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> > > > > nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> > > > >
> > > > > skb->protocol = htons(ETH_P_IPV6);
> > > > > + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > > > > +
> > > > > NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > > skb->dev, NULL,
> > > > > br_nf_pre_routing_finish_ipv6);
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > >
> > > Acked-by: Florian Westphal <fw@strlen.de>
> > >
> > > ... because this fixes discrepancy of normal stack vs. bridge path.
> > >
> > > However, I think we still need a fix for the ipv6 case as
> > > there is no guarantee ipv6 nexthdr will be the sctp one.
> > >
> > > We already pass the sctp offset as argument to sctp_compute_cksum(),
> > > why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
> > > Xin Long originally suggested?
> > >
> > > IOW, whats the problem with
> > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?
> >
> > That patch looks fine too.
> >
> > > (In theory conntrack can also inspect transport header inside icmp error
> > > messages like pkttoobig and so on,
> > > although this won't be the case here as we can't validate csum anyway
> > > if the packet isn't complete).
> > >
> > > Just pointing out that we can't rely on skb transport header being
> > > "correct" in all cases from netfilter point of view.
> >
> > Indeed.
> Hi Neil,
>
> It seems we should bring that patch back, otherwize we will have to add
> a special sctp csum computing function for Netfilter?
>
Fine.
Neil
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-15 11:09 ` Neil Horman
@ 2019-03-15 13:39 ` Xin Long
0 siblings, 0 replies; 8+ messages in thread
From: Xin Long @ 2019-03-15 13:39 UTC (permalink / raw)
To: Neil Horman
Cc: Pablo Neira Ayuso, Florian Westphal, network dev,
netfilter-devel, Marcelo Ricardo Leitner
On Fri, Mar 15, 2019 at 7:10 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Thu, Mar 14, 2019 at 10:41:41PM +0800, Xin Long wrote:
> > On Thu, Mar 14, 2019 at 10:19 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> > > On Wed, Mar 13, 2019 at 12:59:48PM +0100, Florian Westphal wrote:
> > > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > > > > > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > > > > > skb->transport_header has been always set before entering INET
> > > > > > netfilter. This patch is to set skb->transport_header for bridge
> > > > > > before entering INET netfilter by bridge-nf-call-iptables.
> > > > > >
> > > > > > It also fixes an issue that sctp_error() couldn't compute a right
> > > > > > csum due to unset skb->transport_header.
> > > > > >
> > > > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > > > Reported-by: Li Shuang <shuali@redhat.com>
> > > > > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > > net/bridge/br_netfilter_hooks.c | 1 +
> > > > > > net/bridge/br_netfilter_ipv6.c | 2 ++
> > > > > > 2 files changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > > > > > index c93c35b..4d09a33 100644
> > > > > > --- a/net/bridge/br_netfilter_hooks.c
> > > > > > +++ b/net/bridge/br_netfilter_hooks.c
> > > > > > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> > > > > > nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> > > > > >
> > > > > > skb->protocol = htons(ETH_P_IP);
> > > > > > + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> > > > > >
> > > > > > NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > > > skb->dev, NULL,
> > > > > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > > > > > index 564710f..e88d664 100644
> > > > > > --- a/net/bridge/br_netfilter_ipv6.c
> > > > > > +++ b/net/bridge/br_netfilter_ipv6.c
> > > > > > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> > > > > > nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> > > > > >
> > > > > > skb->protocol = htons(ETH_P_IPV6);
> > > > > > + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > > > > > +
> > > > > > NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> > > > > > skb->dev, NULL,
> > > > > > br_nf_pre_routing_finish_ipv6);
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > > >
> > > > Acked-by: Florian Westphal <fw@strlen.de>
> > > >
> > > > ... because this fixes discrepancy of normal stack vs. bridge path.
> > > >
> > > > However, I think we still need a fix for the ipv6 case as
> > > > there is no guarantee ipv6 nexthdr will be the sctp one.
> > > >
> > > > We already pass the sctp offset as argument to sctp_compute_cksum(),
> > > > why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
> > > > Xin Long originally suggested?
> > > >
> > > > IOW, whats the problem with
> > > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?
> > >
> > > That patch looks fine too.
> > >
> > > > (In theory conntrack can also inspect transport header inside icmp error
> > > > messages like pkttoobig and so on,
> > > > although this won't be the case here as we can't validate csum anyway
> > > > if the packet isn't complete).
> > > >
> > > > Just pointing out that we can't rely on skb transport header being
> > > > "correct" in all cases from netfilter point of view.
> > >
> > > Indeed.
> > Hi Neil,
> >
> > It seems we should bring that patch back, otherwize we will have to add
> > a special sctp csum computing function for Netfilter?
> >
> Fine.
I will repost that patch, thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING
2019-03-13 8:33 [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING Xin Long
2019-03-13 11:33 ` Neil Horman
@ 2019-03-18 15:22 ` Pablo Neira Ayuso
1 sibling, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2019-03-18 15:22 UTC (permalink / raw)
To: Xin Long
Cc: network dev, netfilter-devel, Marcelo Ricardo Leitner, Neil Horman, fw
On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> skb->transport_header has been always set before entering INET
> netfilter. This patch is to set skb->transport_header for bridge
> before entering INET netfilter by bridge-nf-call-iptables.
>
> It also fixes an issue that sctp_error() couldn't compute a right
> csum due to unset skb->transport_header.
Applied, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-18 15:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13 8:33 [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE_ROUTING Xin Long
2019-03-13 11:33 ` Neil Horman
2019-03-13 11:59 ` Florian Westphal
2019-03-14 14:19 ` Pablo Neira Ayuso
2019-03-14 14:41 ` Xin Long
2019-03-15 11:09 ` Neil Horman
2019-03-15 13:39 ` Xin Long
2019-03-18 15:22 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).