Noticed when rebasing udp zerocopy. I considered merging as part of that patchset, but this seemed like it should go to net, even if it does not trigger in practice.

Merged net with this patch onto net-next with udp zerocopy to verify that there is no merge conflict.

---
 net/ipv4/ip_output.c  | 3 ++-
 net/ipv6/ip6_output.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

net/ipv4/ip_output.c +2 -1

index c09219e7f230..5dbec21856f4 100644
@@ -939,7 +939,7 @@ static int __ip_append_data(struct sock *sk,
 			unsigned int fraglen;
 			unsigned int fraggap;
 			unsigned int alloclen;
-			unsigned int pagedlen = 0;
+			unsigned int pagedlen;
 			struct sk_buff *skb_prev;
 alloc_new_skb:
 			skb_prev = skb;

@@ -956,6 +956,7 @@ static int __ip_append_data(struct sock *sk,
 			if (datalen > mtu - fragheaderlen)
 				datalen = maxfraglen - fragheaderlen;
 			fraglen = datalen + fragheaderlen;
+			pagedlen = 0;
 
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))

net/ipv6/ip6_output.c +2 -1

index 89e0d5118afe..827a3f5ff3bb 100644
@@ -1354,7 +1354,7 @@ static int __ip6_append_data(struct sock *sk,
 			unsigned int fraglen;
 			unsigned int fraggap;
 			unsigned int alloclen;
-			unsigned int pagedlen = 0;
+			unsigned int pagedlen;
 alloc_new_skb:
 			/* There's no room in the current skb */
 			if (skb)

@@ -1378,6 +1378,7 @@ static int __ip6_append_data(struct sock *sk,
 			if (datalen > (cork->length <= mtu && !(cork->flags & IPCORK_ALLFRAG) ? mtu : maxfraglen) - fragheaderlen)
 				datalen = maxfraglen - fragheaderlen - rt->dst.trailer_len;
 			fraglen = datalen + fragheaderlen;
+			pagedlen = 0;
 
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))

Signature · 1 line

2.20.0.rc0.387.gc7a69e6b6c-goog

David Miller

maintainer

2018-11-25 12:33 UTC #1 lore raw

From: Willem de Bruijn <[email protected]>
Date: Sat, 24 Nov 2018 14:21:16 -0500

Quoted 14 lines

From: Willem de Bruijn <[email protected]>
In ip packet generation, pagedlen is initialized for each skb at the start of the loop in __ip(6)_append_data, before label alloc_new_skb.
Depending on compiler options, code can be generated that jumps to this label, triggering use of an an uninitialized variable.
In practice, at -O2, the generated code moves the initialization below the label. But the code should not rely on that for correctness.
Fixes 15e36f5b8e98 ("udp: paged allocation with gso")
Signed-off-by Willem de Bruijn <[email protected]>

Quoted 8 lines

---
Noticed when rebasing udp zerocopy. I considered merging as part of that patchset, but this seemed like it should go to net, even if it does not trigger in practice.
Merged net with this patch onto net-next with udp zerocopy to verify that there is no merge conflict.

Ok, applied, but not queued for -stable.

Let me know if I should -stable this.

Willem de Bruijn

author

2018-11-25 12:38 UTC #2 lore raw

Quoted 24 lines · David Miller

From: Willem de Bruijn <[email protected]>
Date: Sat, 24 Nov 2018 14:21:16 -0500
Quoted 20 lines
From: Willem de Bruijn <[email protected]>
In ip packet generation, pagedlen is initialized for each skb at the start of the loop in __ip(6)_append_data, before label alloc_new_skb.
Depending on compiler options, code can be generated that jumps to this label, triggering use of an an uninitialized variable.
In practice, at -O2, the generated code moves the initialization below the label. But the code should not rely on that for correctness.
Fixes 15e36f5b8e98 ("udp: paged allocation with gso")
Signed-off-by Willem de Bruijn <[email protected]>
---
Noticed when rebasing udp zerocopy. I considered merging as part of that patchset, but this seemed like it should go to net, even if it does not trigger in practice.

Quoted 6 lines

Quoted 2 lines
Merged net with this patch onto net-next with udp zerocopy to verify that there is no merge conflict.
Ok, applied, but not queued for -stable.
Let me know if I should -stable this.

Thanks, David. No, that's not needed.