On Tue, 2 Oct 2012, Linux Kernel Mailing List wrote:
> Gitweb: http://git.kernel.org/linus/;a=commit;h=c12b395a46646bab69089ce7016ac78177f6001f
> Commit: c12b395a46646bab69089ce7016ac78177f6001f
> Parent: b7bc2a5b5bd99b216c3e5fe68c7f45c684ab5745
> Author: [email protected] <[email protected]>
> AuthorDate: Fri Aug 10 00:51:50 2012 +0000
> Committer: David S. Miller <[email protected]>
> CommitDate: Tue Aug 14 14:28:32 2012 -0700
>
> gre: Support GRE over IPv6
>
> GRE over IPv6 implementation.
>
> Signed-off-by: Dmitry Kozlov <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> --- /dev/null
> +++ b/net/ipv6/ip6_gre.c
> +static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
> + struct net_device *dev,
> + __u8 dsfield,
> + struct flowi6 *fl6,
> + int encap_limit,
> + __u32 *pmtu)
> +{
> + struct net *net = dev_net(dev);
> + struct ip6_tnl *tunnel = netdev_priv(dev);
> + struct net_device *tdev; /* Device to other host */
> + struct ipv6hdr *ipv6h; /* Our new IP header */
> + unsigned int max_headroom; /* The extra header space needed */
max_headroom is not initialized
> + mtu = dst_mtu(dst) - sizeof(*ipv6h);
> + if (encap_limit >= 0) {
> + max_headroom += 8;
Hence gcc (4.1.2) rightfully complains:
net/ipv6/ip6_gre.c: In function ‘ip6gre_xmit2’:
net/ipv6/ip6_gre.c:713: warning: ‘max_headroom’ is used uninitialized in this function
However, initializing max_headroom to zero at the top, or replacing the line
above by "max_headroom = 8" doesn't seem to be the right fix...
> + mtu -= 8;
> + }
> + if (mtu < IPV6_MIN_MTU)
> + mtu = IPV6_MIN_MTU;
> + if (skb_dst(skb))
> + skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
> + if (skb->len > mtu) {
> + *pmtu = mtu;
> + err = -EMSGSIZE;
> + goto tx_err_dst_release;
> + }
> +
> + if (tunnel->err_count > 0) {
> + if (time_before(jiffies,
> + tunnel->err_time + IP6TUNNEL_ERR_TIMEO)) {
> + tunnel->err_count--;
> +
> + dst_link_failure(skb);
> + } else
> + tunnel->err_count = 0;
> + }
> +
> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + dst->header_len;
... as max_headroom is overwritten here anyway?
So something is really wrong here. What is the right fix?
This issue is present in current mainline as of v3.7.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 2013-04-24 at 14:23 +0200, Geert Uytterhoeven wrote:
> On Tue, 2 Oct 2012, Linux Kernel Mailing List wrote:
> > Gitweb: http://git.kernel.org/linus/;a=commit;h=c12b395a46646bab69089ce7016ac78177f6001f
> > Commit: c12b395a46646bab69089ce7016ac78177f6001f
> > Parent: b7bc2a5b5bd99b216c3e5fe68c7f45c684ab5745
> > Author: [email protected] <[email protected]>
> > AuthorDate: Fri Aug 10 00:51:50 2012 +0000
> > Committer: David S. Miller <[email protected]>
> > CommitDate: Tue Aug 14 14:28:32 2012 -0700
> >
> > gre: Support GRE over IPv6
> >
> > GRE over IPv6 implementation.
> >
> > Signed-off-by: Dmitry Kozlov <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
>
> > --- /dev/null
> > +++ b/net/ipv6/ip6_gre.c
>
> > +static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
> > + struct net_device *dev,
> > + __u8 dsfield,
> > + struct flowi6 *fl6,
> > + int encap_limit,
> > + __u32 *pmtu)
> > +{
> > + struct net *net = dev_net(dev);
> > + struct ip6_tnl *tunnel = netdev_priv(dev);
> > + struct net_device *tdev; /* Device to other host */
> > + struct ipv6hdr *ipv6h; /* Our new IP header */
> > + unsigned int max_headroom; /* The extra header space needed */
>
> max_headroom is not initialized
>
> > + mtu = dst_mtu(dst) - sizeof(*ipv6h);
> > + if (encap_limit >= 0) {
> > + max_headroom += 8;
>
> Hence gcc (4.1.2) rightfully complains:
>
> net/ipv6/ip6_gre.c: In function ‘ip6gre_xmit2’:
> net/ipv6/ip6_gre.c:713: warning: ‘max_headroom’ is used uninitialized in this function
>
> However, initializing max_headroom to zero at the top, or replacing the line
> above by "max_headroom = 8" doesn't seem to be the right fix...
>
> > + mtu -= 8;
> > + }
> > + if (mtu < IPV6_MIN_MTU)
> > + mtu = IPV6_MIN_MTU;
> > + if (skb_dst(skb))
> > + skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
> > + if (skb->len > mtu) {
> > + *pmtu = mtu;
> > + err = -EMSGSIZE;
> > + goto tx_err_dst_release;
> > + }
> > +
> > + if (tunnel->err_count > 0) {
> > + if (time_before(jiffies,
> > + tunnel->err_time + IP6TUNNEL_ERR_TIMEO)) {
> > + tunnel->err_count--;
> > +
> > + dst_link_failure(skb);
> > + } else
> > + tunnel->err_count = 0;
> > + }
> > +
> > + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + dst->header_len;
>
> ... as max_headroom is overwritten here anyway?
>
> So something is really wrong here. What is the right fix?
>
> This issue is present in current mainline as of v3.7.
I would use following fix. Can you provide an official patch ?
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index d3ddd84..150d17d 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -620,7 +620,7 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
struct ip6_tnl *tunnel = netdev_priv(dev);
struct net_device *tdev; /* Device to other host */
struct ipv6hdr *ipv6h; /* Our new IP header */
- unsigned int max_headroom; /* The extra header space needed */
+ unsigned int max_headroom = 0; /* The extra header space needed */
int gre_hlen;
struct ipv6_tel_txoption opt;
int mtu;
@@ -693,7 +693,7 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
tunnel->err_count = 0;
}
- max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + dst->header_len;
+ max_headroom += LL_RESERVED_SPACE(tdev) + gre_hlen + dst->header_len;
if (skb_headroom(skb) < max_headroom || skb_shared(skb) ||
(skb_cloned(skb) && !skb_clone_writable(skb, 0))) {