Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum

[Xin Long <lucien.xin@gmail.com>]

On Tue, Mar 12, 2019 at 5:49 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Mar 12, 2019 at 04:39:46PM +0800, Xin Long wrote:
> > On Mon, Mar 11, 2019 at 7:08 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Sat, Mar 09, 2019 at 10:24:34AM +0100, Florian Westphal wrote:
> > > > Xin Long <lucien.xin@gmail.com> wrote:
> > > > >   https://marc.info/?l=linux-netdev&m=155109395226858&w=2
> > > > > But from sctp side, Neil preferred sctp_hdr().
> > > > >
> > > > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler()
> > > > > and sctp_manip_pkt(), or bring that patch back?
> > > > >
> > > > > Now it seems not good to set skb->transport_header in netfilter code.
> > > >
> > > > I think its fine, but I wonder why we need to do it.
> > > >
> > > > Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets
> > > > transport header before netfilter.  The only problem is that linear
> > > > access is illegal without may_pull checks, but in this case the
> > > > make_writable call takes care of this already.
> > > >
> > > Yes, this.  It seems to me we should be setting the transport header prior to
> > > ever getting into the netfilter code, which does imply that we need the may_pull
> > > check to linearize enough of the packet to do so, just like tcp and udp do.
> > >
> > > > So, why was this patch needed?
> >
> > The issue was reported when going to nf_conntrack by br_netfilter's
> > bridge-nf-call-iptables, which could be:
> >
> > br_prerouting->inet_prerouting->
> > br_forward->inet_forward->
> > br_postrouting->inet_postrouting
>
> Can you fix this from br_netfilter then? ie. set the transport header
> before prerouting to emulate the IP stack behaviour.
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 9d34de6..22afa56 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);

Looks more reasonable, it's also safe after br_validate_ipv4/6(). Thanks.