diff mbox series

[net] ip: Fix SO_MARK in RST, ACK and ICMP packets

Message ID 20200701200006.2414835-1-willemdebruijn.kernel@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [net] ip: Fix SO_MARK in RST, ACK and ICMP packets | expand

Commit Message

Willem de Bruijn July 1, 2020, 8 p.m. UTC 
From: Willem de Bruijn <willemb@google.com>

When no full socket is available, skbs are sent over a per-netns
control socket. Its sk_mark is temporarily adjusted to match that
of the real (request or timewait) socket or to reflect an incoming
skb, so that the outgoing skb inherits this in __ip_make_skb.

Introduction of the socket cookie mark field broke this. Now the
skb is set through the cookie and cork:

<caller>		# init sockc.mark from sk_mark or cmsg
ip_append_data
  ip_setup_cork		# convert sockc.mark to cork mark
ip_push_pending_frames
  ip_finish_skb
    __ip_make_skb	# set skb->mark to cork mark

But I missed these special control sockets. Update all callers of
__ip(6)_make_skb that were originally missed.

For IPv6, the same two icmp(v6) paths are affected. The third
case is not, as commit 92e55f412cff ("tcp: don't annotate
mark on control socket from tcp_v6_send_response()") replaced
the ctl_sk->sk_mark with passing the mark field directly as a
function argument. That commit predates the commit that
introduced the bug.

Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
Signed-off-by: Willem de Bruijn <willemb@google.com>
Reported-by: Martin KaFai Lau <kafai@fb.com>
---
 net/ipv4/icmp.c      | 4 ++--
 net/ipv4/ip_output.c | 2 +-
 net/ipv6/icmp.c      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

Willem de Bruijn July 1, 2020, 9:26 p.m. UTC | #1 
On Wed, Jul 1, 2020 at 4:00 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> When no full socket is available, skbs are sent over a per-netns
> control socket. Its sk_mark is temporarily adjusted to match that
> of the real (request or timewait) socket or to reflect an incoming
> skb, so that the outgoing skb inherits this in __ip_make_skb.
>
> Introduction of the socket cookie mark field broke this. Now the
> skb is set through the cookie and cork:
>
> <caller>                # init sockc.mark from sk_mark or cmsg
> ip_append_data
>   ip_setup_cork         # convert sockc.mark to cork mark
> ip_push_pending_frames
>   ip_finish_skb
>     __ip_make_skb       # set skb->mark to cork mark
>
> But I missed these special control sockets. Update all callers of
> __ip(6)_make_skb that were originally missed.
>
> For IPv6, the same two icmp(v6) paths are affected. The third
> case is not, as commit 92e55f412cff ("tcp: don't annotate
> mark on control socket from tcp_v6_send_response()") replaced
> the ctl_sk->sk_mark with passing the mark field directly as a
> function argument. That commit predates the commit that
> introduced the bug.
>
> Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Reported-by: Martin KaFai Lau <kafai@fb.com>

I spotted another missing case, in ping_v6_sendmsg. Will have to send a v2.
Willem de Bruijn July 1, 2020, 9:49 p.m. UTC | #2 
On Wed, Jul 1, 2020 at 5:26 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, Jul 1, 2020 at 4:00 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.com>
> >
> > When no full socket is available, skbs are sent over a per-netns
> > control socket. Its sk_mark is temporarily adjusted to match that
> > of the real (request or timewait) socket or to reflect an incoming
> > skb, so that the outgoing skb inherits this in __ip_make_skb.
> >
> > Introduction of the socket cookie mark field broke this. Now the
> > skb is set through the cookie and cork:
> >
> > <caller>                # init sockc.mark from sk_mark or cmsg
> > ip_append_data
> >   ip_setup_cork         # convert sockc.mark to cork mark
> > ip_push_pending_frames
> >   ip_finish_skb
> >     __ip_make_skb       # set skb->mark to cork mark
> >
> > But I missed these special control sockets. Update all callers of
> > __ip(6)_make_skb that were originally missed.
> >
> > For IPv6, the same two icmp(v6) paths are affected. The third
> > case is not, as commit 92e55f412cff ("tcp: don't annotate
> > mark on control socket from tcp_v6_send_response()") replaced
> > the ctl_sk->sk_mark with passing the mark field directly as a
> > function argument. That commit predates the commit that
> > introduced the bug.
> >
> > Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > Reported-by: Martin KaFai Lau <kafai@fb.com>
>
> I spotted another missing case, in ping_v6_sendmsg. Will have to send a v2.

Turns out, that case is indeed missing, but never existed in the first place.
I can send a separate patch to net-next to add it.

That means that this patch is good as is.
Martin KaFai Lau July 1, 2020, 9:58 p.m. UTC | #3 
On Wed, Jul 01, 2020 at 04:00:06PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> When no full socket is available, skbs are sent over a per-netns
> control socket. Its sk_mark is temporarily adjusted to match that
> of the real (request or timewait) socket or to reflect an incoming
> skb, so that the outgoing skb inherits this in __ip_make_skb.
> 
> Introduction of the socket cookie mark field broke this. Now the
> skb is set through the cookie and cork:
> 
> <caller>		# init sockc.mark from sk_mark or cmsg
> ip_append_data
>   ip_setup_cork		# convert sockc.mark to cork mark
> ip_push_pending_frames
>   ip_finish_skb
>     __ip_make_skb	# set skb->mark to cork mark
> 
> But I missed these special control sockets. Update all callers of
> __ip(6)_make_skb that were originally missed.
Reviewed-by: Martin KaFai Lau <kafai@fb.com>
David Miller July 2, 2020, 12:39 a.m. UTC | #4 
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed,  1 Jul 2020 16:00:06 -0400

> From: Willem de Bruijn <willemb@google.com>
> 
> When no full socket is available, skbs are sent over a per-netns
> control socket. Its sk_mark is temporarily adjusted to match that
> of the real (request or timewait) socket or to reflect an incoming
> skb, so that the outgoing skb inherits this in __ip_make_skb.
> 
> Introduction of the socket cookie mark field broke this. Now the
> skb is set through the cookie and cork:
> 
> <caller>		# init sockc.mark from sk_mark or cmsg
> ip_append_data
>   ip_setup_cork		# convert sockc.mark to cork mark
> ip_push_pending_frames
>   ip_finish_skb
>     __ip_make_skb	# set skb->mark to cork mark
> 
> But I missed these special control sockets. Update all callers of
> __ip(6)_make_skb that were originally missed.
> 
> For IPv6, the same two icmp(v6) paths are affected. The third
> case is not, as commit 92e55f412cff ("tcp: don't annotate
> mark on control socket from tcp_v6_send_response()") replaced
> the ctl_sk->sk_mark with passing the mark field directly as a
> function argument. That commit predates the commit that
> introduced the bug.
> 
> Fixes: c6af0c227a22 ("ip: support SO_MARK cmsg")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Reported-by: Martin KaFai Lau <kafai@fb.com>

Applied and queued up for -stable, thanks.
diff mbox series

Patch

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 956a806649f7..e30515f89802 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -427,7 +427,7 @@  static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 
 	ipcm_init(&ipc);
 	inet->tos = ip_hdr(skb)->tos;
-	sk->sk_mark = mark;
+	ipc.sockc.mark = mark;
 	daddr = ipc.addr = ip_hdr(skb)->saddr;
 	saddr = fib_compute_spec_dst(skb);
 
@@ -710,10 +710,10 @@  void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
 	inet_sk(sk)->tos = tos;
-	sk->sk_mark = mark;
 	ipcm_init(&ipc);
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts.opt;
+	ipc.sockc.mark = mark;
 
 	rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos, mark,
 			       type, code, &icmp_param);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 090d3097ee15..17206677d503 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1702,7 +1702,7 @@  void ip_send_unicast_reply(struct sock *sk, struct sk_buff *skb,
 	sk->sk_protocol = ip_hdr(skb)->protocol;
 	sk->sk_bound_dev_if = arg->bound_dev_if;
 	sk->sk_sndbuf = sysctl_wmem_default;
-	sk->sk_mark = fl4.flowi4_mark;
+	ipc.sockc.mark = fl4.flowi4_mark;
 	err = ip_append_data(sk, &fl4, ip_reply_glue_bits, arg->iov->iov_base,
 			     len, 0, &ipc, &rt, MSG_DONTWAIT);
 	if (unlikely(err)) {
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index fc5000370030..9df8737ae0d3 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -566,7 +566,6 @@  static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
 	fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb, NULL);
 	security_skb_classify_flow(skb, flowi6_to_flowi(&fl6));
 
-	sk->sk_mark = mark;
 	np = inet6_sk(sk);
 
 	if (!icmpv6_xrlim_allow(sk, type, &fl6))
@@ -583,6 +582,7 @@  static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
 		fl6.flowi6_oif = np->ucast_oif;
 
 	ipcm6_init_sk(&ipc6, np);
+	ipc6.sockc.mark = mark;
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
 	dst = icmpv6_route_lookup(net, skb, sk, &fl6);
@@ -751,7 +751,6 @@  static void icmpv6_echo_reply(struct sk_buff *skb)
 	sk = icmpv6_xmit_lock(net);
 	if (!sk)
 		goto out_bh_enable;
-	sk->sk_mark = mark;
 	np = inet6_sk(sk);
 
 	if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr))
@@ -779,6 +778,7 @@  static void icmpv6_echo_reply(struct sk_buff *skb)
 	ipcm6_init_sk(&ipc6, np);
 	ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
 	ipc6.tclass = ipv6_get_dsfield(ipv6_hdr(skb));
+	ipc6.sockc.mark = mark;
 
 	if (ip6_append_data(sk, icmpv6_getfrag, &msg,
 			    skb->len + sizeof(struct icmp6hdr),