linux-kernel - Re: {painfully BISECTED} Please revert f25c80a4b2: arch/um/drivers: remove duplicate structure field initialization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20100928134721.f79a5955.akpm@linux-foundation.org>
Date:	Tue, 28 Sep 2010 13:47:21 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Boaz Harrosh <bharrosh@...asas.com>, Julia Lawall <julia@...u.dk>,
	"David S. Miller" <davem@...emloft.net>,
	uml-devel <user-mode-linux-devel@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Stephen Hemminger <shemminger@...tta.com>
Subject: Re: {painfully BISECTED} Please revert f25c80a4b2: arch/um/drivers:
 remove duplicate structure field initialization

On Tue, 28 Sep 2010 13:24:40 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> Ping, no comments?
> 
> On Mon, Sep 27, 2010 at 6:17 AM, Boaz Harrosh <bharrosh@...asas.com> wrote:
> >
> > [bharrosh@fs2 ~/dev/git/pub/scsi-misc] 1115$ git bisect good
> > f25c80a4b2bf93c99820f470573626557db35202 is the first bad commit
> > commit f25c80a4b2bf93c99820f470573626557db35202
> 
> It looks like that commit is indeed very misleading. The commit message says:
> 
>   "arch/um/drivers: remove duplicate structure field initialization"
> 
> but it is in fact not duplicate: there's two field initializations,
> but they are _different_. Looking at the patch, it has:
> 
>         .ndo_set_mac_address    = uml_net_set_mac,
> -       .ndo_set_mac_address    = eth_mac_addr,
> 
> so it removes the later one, but it is not at all clear which one the
> compiler actually used. My guess is that it used to use the later one
> (the standard eth_mac_addr function), and the patch made it suddenly
> use the uml_net_set_mac function.
> 
> I didn't check what gcc used to do, but this:
> 
> > The patch Reverts cleanly on top of 2.6.36-rc5 and after Revert works perfectly as
> > before.
> 
> makes me suspect that nobody else checked it either.

I checked! gcc uses the second initialiser.

uml_net_set_mac() is:

static int uml_net_set_mac(struct net_device *dev, void *addr)
{
	struct uml_net_private *lp = netdev_priv(dev);
	struct sockaddr *hwaddr = addr;

	spin_lock_irq(&lp->lock);
	eth_mac_addr(dev, hwaddr->sa_data);
	spin_unlock_irq(&lp->lock);

	return 0;
}

And I misread that, assuming that it's just a wrapper around
eth_mac_addr().  Only it isn't, because it passes eth_mac_addr() the
MAC address's address directly (with ->sa_data).  But eth_mac_addr()
expects a `struct sockaddr *'.

And for some wtf reason, eth_mac_addr() passes that `struct
sockaddr *' in a `void *', thus cunningly hiding the bug.

Yeah, please revert it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/