[PATCH] fix kernel crash in the macvlan driver

[PATCH] fix kernel crash in the macvlan driver

[Ani Sinha]

Hello folks :

We noticed a consistently reproducable kernel crash in the macvlan driver
when we were running our test script. I believe I have found the reason
for the crash and a patch that fixes it. I am attaching the patch for your
comments and opinions.

Cheers,
Ani



commit cd28ce3cb624ddaaf97935c1f34d44bb13ffb786
Author: Anirban Sinha <ani@aristanetworks.com>
Date:   Thu Jun 7 11:21:02 2012 -0700

    macvlan : The patch d5cd92448fded12c91f7574e49747c5f7d975a8d introduced reference
    counting for macvlan_port. This patch fixes an issue where the reference
    counts were being decremented incorrectly from macvlan_uninit() and not from
    macvlan_dellink(). This was causing the kernel crash shown below :

    BUG: unable to handle kernel paging request at 0000000100000000
    IP: [<ffffffffa031f8e5>] macvlan_addr_busy+0x58/0x8d [macvlan]
    PGD 3a2aa067 PUD 0
    Oops: 0000 [#1] SMP
    last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1/energy_full
    CPU 0
    Modules linked in: macvlan rfcomm sco bnep l2cap sunrpc ipt_REJECT iptable_filter ip6t_REJECT xt_tcpudp

    Pid: 2490, comm: ip Not tainted 2.6.38.8-705892.2012aniArora.7.fc14.x86_64 #1
    RIP: 0010:[<ffffffffa031f8e5>]  [<ffffffffa031f8e5>] macvlan_addr_busy+0x58/0x8d [macvlan]
    RSP: 0018:ffff880037d2b698  EFLAGS: 00010296
    RAX: 0000000100000000 RBX: ffff88003bf54000 RCX: 0000000000000000
    RDX: 0000111111111102 RSI: 0000000000000092 RDI: 0000000000000246
    RBP: ffff880037d2b6a8 R08: 0000000000000040 R09: ffffffff81a73f18
    R10: ffffffff81e03a20 R11: 0000000000000020 R12: ffff88003c8e33d0
    R13: ffff880037ecd000 R14: 00000000fffffff0 R15: 0000000000000001
    FS:  0000000000000000(0000) GS:ffff88003e200000(0063) knlGS:00000000f75d26c0
    CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
    CR2: 0000000100000000 CR3: 00000000377e7000 CR4: 00000000000006f0
    DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
    Process ip (pid: 2490, threadinfo ffff880037d2a000, task ffff88003d1206c0)
    Stack:
     ffff88003d031000 ffff88003d031680 ffff880037d2b6d8 ffffffffa031fbcc
     ffff88003d031000 ffffffffa03211c0 ffff88003d031078 0000000000000000
     ffff880037d2b708 ffffffff81345cf7 ffff88003d031000 0000000000001003
    Call Trace:
     [<ffffffffa031fbcc>] macvlan_open+0x7e/0x116 [macvlan]
     [<ffffffff81345cf7>] __dev_open+0x90/0xc7
     [<ffffffff81345f26>] __dev_change_flags+0xa8/0x12c
     [<ffffffff81346021>] dev_change_flags+0x1c/0x52
     [<ffffffff8134fce1>] do_setlink+0x2b4/0x67d
     [<ffffffff813bc85f>] ? inet6_fill_link_af+0x1a/0x22
     [<ffffffff8134f6bf>] ? rtnl_fill_ifinfo+0x99f/0xa7d
     [<ffffffff81350d02>] rtnl_newlink+0x247/0x40d
     [<ffffffff81350b7a>] ? rtnl_newlink+0xbf/0x40d
     [<ffffffff81337ced>] ? sock_rmalloc+0x2e/0x90
     [<ffffffff810dc46c>] ? arch_local_irq_save+0x16/0x1c
     [<ffffffff810685fe>] ? arch_local_irq_save+0x18/0x1e
     [<ffffffff813508dc>] rtnetlink_rcv_msg+0x1e6/0x1fc
     [<ffffffff810af70a>] ? get_page_from_freelist+0x4dd/0x68d
     [<ffffffff813506f6>] ? rtnetlink_rcv_msg+0x0/0x1fc
     [<ffffffff81364239>] netlink_rcv_skb+0x40/0x8b
     [<ffffffff813502c7>] rtnetlink_rcv+0x21/0x28
     [<ffffffff81363d27>] netlink_unicast+0xec/0x155
     [<ffffffff81364041>] netlink_sendmsg+0x2b1/0x2cf
     [<ffffffff81332ec2>] ? __sock_recvmsg+0x75/0x84
     [<ffffffff81332d70>] __sock_sendmsg+0x66/0x72
     [<ffffffff81333521>] sock_sendmsg+0xa3/0xbc
     [<ffffffff810b25da>] ? lru_cache_add_lru+0x3c/0x3e
     [<ffffffff810cc1e3>] ? page_add_new_anon_rmap+0x5b/0x6d
     [<ffffffff810c17cb>] ? set_pte_at+0x9/0xd
     [<ffffffff810c2f31>] ? do_wp_page+0x496/0x541
     [<ffffffff81333ec0>] ? move_addr_to_kernel+0x44/0x49
     [<ffffffff813585a4>] ? verify_compat_iovec+0x6d/0xb9
     [<ffffffff81334cac>] sys_sendmsg+0x230/0x2ae
     [<ffffffff810c1965>] ? pmd_offset+0x14/0x3b
     [<ffffffff810c4c0a>] ? handle_mm_fault+0x13a/0x14f
     [<ffffffff81334756>] ? sys_sendto+0x13f/0x16c
     [<ffffffff81334d76>] ? sys_recvmsg+0x4c/0x5b
     [<ffffffff81358deb>] compat_sys_sendmsg+0xf/0x11
     [<ffffffff81359005>] compat_sys_socketcall+0x14f/0x186
     [<ffffffff8102c2b0>] sysenter_dispatch+0x7/0x2e
    Code: 0e e1 48 8b 03 4c 89 e2 48 c7 c7 6c 10 32 a0 48 8b b0 80 02 00 00 31 c0 e8 f1 17 0e e1 48 8b 03 49 8b 14 24 48 8b 80 80 02 00 00 <48> 33 10 b8 01 00 00 00 48 c1 e2 10 74 22 48 c7 c7 93 10 32 a0

    Signed-off-by: Anirban Sinha <ani@aristanetworks.com>

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 66a9bfe..d880bc8 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -481,7 +481,6 @@ static void macvlan_uninit(struct net_device *dev)

 	free_percpu(vlan->pcpu_stats);

-	port->count -= 1;
 	if (!port->count)
 		macvlan_port_destroy(port->dev);
 }
@@ -795,7 +794,9 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
 void macvlan_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct macvlan_port *port = vlan->port;

+	port->count -= 1;
 	list_del(&vlan->list);
 	unregister_netdevice_queue(dev, head);
 }

Re: [PATCH] fix kernel crash in the macvlan driver

[Eric W. Biederman]

Ani Sinha <ani@aristanetworks.com> writes:

> Hello folks :
>
> We noticed a consistently reproducable kernel crash in the macvlan driver
> when we were running our test script. I believe I have found the reason
> for the crash and a patch that fixes it. I am attaching the patch for your
> comments and opinions.

I don't completely follow the logic of your change.  Crashing in
macvlan_addr_busy does seem to indicate you are using a corrupted data
structure.

My compiled version of macvlan_addr_busy is much smaller than yours so I
can't guess based on your disassembly what is wrong.  But by reading the
code it must either be port->dev->dev_addr or the rcu
macvlan_hash_lookup.

Regardless all I see your patch doing is moving the decrement of
port->count earlier and possibly allowing newlink in
MACVLAN_MODE_PASSTHRU to succeed a smidge earlier.

I might just be dense today but I can't possibly see how moving that
decrement would solve the crash you have reported below.

Eric


> commit cd28ce3cb624ddaaf97935c1f34d44bb13ffb786
> Author: Anirban Sinha <ani@aristanetworks.com>
> Date:   Thu Jun 7 11:21:02 2012 -0700
>
>     macvlan : The patch d5cd92448fded12c91f7574e49747c5f7d975a8d introduced reference
>     counting for macvlan_port. This patch fixes an issue where the reference
>     counts were being decremented incorrectly from macvlan_uninit() and not from
>     macvlan_dellink(). This was causing the kernel crash shown below :
>
>     BUG: unable to handle kernel paging request at 0000000100000000
>     IP: [<ffffffffa031f8e5>] macvlan_addr_busy+0x58/0x8d [macvlan]
>     PGD 3a2aa067 PUD 0
>     Oops: 0000 [#1] SMP
>     last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1/energy_full
>     CPU 0
>     Modules linked in: macvlan rfcomm sco bnep l2cap sunrpc ipt_REJECT iptable_filter ip6t_REJECT xt_tcpudp
>
>     Pid: 2490, comm: ip Not tainted 2.6.38.8-705892.2012aniArora.7.fc14.x86_64 #1
>     RIP: 0010:[<ffffffffa031f8e5>]  [<ffffffffa031f8e5>] macvlan_addr_busy+0x58/0x8d [macvlan]
>     RSP: 0018:ffff880037d2b698  EFLAGS: 00010296
>     RAX: 0000000100000000 RBX: ffff88003bf54000 RCX: 0000000000000000
>     RDX: 0000111111111102 RSI: 0000000000000092 RDI: 0000000000000246
>     RBP: ffff880037d2b6a8 R08: 0000000000000040 R09: ffffffff81a73f18
>     R10: ffffffff81e03a20 R11: 0000000000000020 R12: ffff88003c8e33d0
>     R13: ffff880037ecd000 R14: 00000000fffffff0 R15: 0000000000000001
>     FS:  0000000000000000(0000) GS:ffff88003e200000(0063) knlGS:00000000f75d26c0
>     CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
>     CR2: 0000000100000000 CR3: 00000000377e7000 CR4: 00000000000006f0
>     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>     DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>     Process ip (pid: 2490, threadinfo ffff880037d2a000, task ffff88003d1206c0)
>     Stack:
>      ffff88003d031000 ffff88003d031680 ffff880037d2b6d8 ffffffffa031fbcc
>      ffff88003d031000 ffffffffa03211c0 ffff88003d031078 0000000000000000
>      ffff880037d2b708 ffffffff81345cf7 ffff88003d031000 0000000000001003
>     Call Trace:
>      [<ffffffffa031fbcc>] macvlan_open+0x7e/0x116 [macvlan]
>      [<ffffffff81345cf7>] __dev_open+0x90/0xc7
>      [<ffffffff81345f26>] __dev_change_flags+0xa8/0x12c
>      [<ffffffff81346021>] dev_change_flags+0x1c/0x52
>      [<ffffffff8134fce1>] do_setlink+0x2b4/0x67d
>      [<ffffffff813bc85f>] ? inet6_fill_link_af+0x1a/0x22
>      [<ffffffff8134f6bf>] ? rtnl_fill_ifinfo+0x99f/0xa7d
>      [<ffffffff81350d02>] rtnl_newlink+0x247/0x40d
>      [<ffffffff81350b7a>] ? rtnl_newlink+0xbf/0x40d
>      [<ffffffff81337ced>] ? sock_rmalloc+0x2e/0x90
>      [<ffffffff810dc46c>] ? arch_local_irq_save+0x16/0x1c
>      [<ffffffff810685fe>] ? arch_local_irq_save+0x18/0x1e
>      [<ffffffff813508dc>] rtnetlink_rcv_msg+0x1e6/0x1fc
>      [<ffffffff810af70a>] ? get_page_from_freelist+0x4dd/0x68d
>      [<ffffffff813506f6>] ? rtnetlink_rcv_msg+0x0/0x1fc
>      [<ffffffff81364239>] netlink_rcv_skb+0x40/0x8b
>      [<ffffffff813502c7>] rtnetlink_rcv+0x21/0x28
>      [<ffffffff81363d27>] netlink_unicast+0xec/0x155
>      [<ffffffff81364041>] netlink_sendmsg+0x2b1/0x2cf
>      [<ffffffff81332ec2>] ? __sock_recvmsg+0x75/0x84
>      [<ffffffff81332d70>] __sock_sendmsg+0x66/0x72
>      [<ffffffff81333521>] sock_sendmsg+0xa3/0xbc
>      [<ffffffff810b25da>] ? lru_cache_add_lru+0x3c/0x3e
>      [<ffffffff810cc1e3>] ? page_add_new_anon_rmap+0x5b/0x6d
>      [<ffffffff810c17cb>] ? set_pte_at+0x9/0xd
>      [<ffffffff810c2f31>] ? do_wp_page+0x496/0x541
>      [<ffffffff81333ec0>] ? move_addr_to_kernel+0x44/0x49
>      [<ffffffff813585a4>] ? verify_compat_iovec+0x6d/0xb9
>      [<ffffffff81334cac>] sys_sendmsg+0x230/0x2ae
>      [<ffffffff810c1965>] ? pmd_offset+0x14/0x3b
>      [<ffffffff810c4c0a>] ? handle_mm_fault+0x13a/0x14f
>      [<ffffffff81334756>] ? sys_sendto+0x13f/0x16c
>      [<ffffffff81334d76>] ? sys_recvmsg+0x4c/0x5b
>      [<ffffffff81358deb>] compat_sys_sendmsg+0xf/0x11
>      [<ffffffff81359005>] compat_sys_socketcall+0x14f/0x186
>      [<ffffffff8102c2b0>] sysenter_dispatch+0x7/0x2e
>     Code: 0e e1 48 8b 03 4c 89 e2 48 c7 c7 6c 10 32 a0 48 8b b0 80 02 00 00 31 c0 e8 f1 17 0e e1 48 8b 03 49 8b 14 24 48 8b 80 80 02 00 00 <48> 33 10 b8 01 00 00 00 48 c1 e2 10 74 22 48 c7 c7 93 10 32 a0
>
>     Signed-off-by: Anirban Sinha <ani@aristanetworks.com>
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 66a9bfe..d880bc8 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -481,7 +481,6 @@ static void macvlan_uninit(struct net_device *dev)
>
>  	free_percpu(vlan->pcpu_stats);
>
> -	port->count -= 1;
>  	if (!port->count)
>  		macvlan_port_destroy(port->dev);
>  }
> @@ -795,7 +794,9 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,
>  void macvlan_dellink(struct net_device *dev, struct list_head *head)
>  {
>  	struct macvlan_dev *vlan = netdev_priv(dev);
> +	struct macvlan_port *port = vlan->port;
>
> +	port->count -= 1;
>  	list_del(&vlan->list);
>  	unregister_netdevice_queue(dev, head);
>  }

Re: [PATCH] fix kernel crash in the macvlan driver

[Ani Sinha]

Hi Eric :

On Thu, 7 Jun 2012, Eric W. Biederman wrote:

> I don't completely follow the logic of your change.  Crashing in
> macvlan_addr_busy does seem to indicate you are using a corrupted data
> structure.

The logic of my change is as follows :

As far as I can see, macvlan_newlink() pairs with macvlan_dellink(). If
you are incrementing the reference count in newlink(), the corresponding
decrement should be, in my opinion in dellink(). If you are derementing
the count in uninit(), you are asuming that for every dellink() call,
there is a corresponding uninit() call. I am not sure if this assumption
is correct. Perhaps you can shed some more lights on this.

Now since, macvlan_common_newlink() symbol has been exported but dellink() is not, it
is possible to call the common_newlink() from some GPL driver code and
increment the reference count which will not have a corresponding
decrement. I am not sure what can be done about this issue either.

>
> My compiled version of macvlan_addr_busy is much smaller than yours so I
> can't guess based on your disassembly what is wrong.  But by reading the
> code it must either be port->dev->dev_addr or the rcu
> macvlan_hash_lookup.

Yes, the corruption is in port->dev->dev_addr. The dev_addr seems to get a
bogus address value.


> I might just be dense today but I can't possibly see how moving that
> decrement would solve the crash you have reported below.

In my tests, I have confirmed that with my change, the crash I reported is
no longer reproducable with our scripts. I have also verified that when I
pull out your d5cd92448fded change, I can also no longer reproduce the
issue. So I believe that the crash is related to the above change.
However, I am not very familier with the code in the macvlan
driver, so I can not say for sure that the fix I made genuinely solves the
problem.

Cheers,
Ani

Re: [PATCH] fix kernel crash in the macvlan driver

[Ani Sinha]

>
> Now since, macvlan_common_newlink() symbol has been exported but dellink() is not, it

oops, sorry. Not sure what I was looking at. dellink() has also been
exported and is used by macvtap.c. So no issues here.

ani

Re: [PATCH] fix kernel crash in the macvlan driver

[Eric W. Biederman]

Ani Sinha <ani@aristanetworks.com> writes:

> Hi Eric :
>
> On Thu, 7 Jun 2012, Eric W. Biederman wrote:
>
>> I don't completely follow the logic of your change.  Crashing in
>> macvlan_addr_busy does seem to indicate you are using a corrupted data
>> structure.
>
> The logic of my change is as follows :
>
> As far as I can see, macvlan_newlink() pairs with macvlan_dellink(). If
> you are incrementing the reference count in newlink(), the corresponding
> decrement should be, in my opinion in dellink(). If you are derementing
> the count in uninit(), you are asuming that for every dellink() call,
> there is a corresponding uninit() call. I am not sure if this assumption
> is correct. Perhaps you can shed some more lights on this.

Yes.  Look at net/core/dev.c
dellink calls unregister_netdevice_queue.
The active part of unregister_netdevice_queue rollback_registered_many
calls dev->ndo_stop() and then ndo_uninit.

We might still be using rcu hash lookups until ndo_close is called and
so we really don't want to move the decrement before then.

>> My compiled version of macvlan_addr_busy is much smaller than yours so I
>> can't guess based on your disassembly what is wrong.  But by reading the
>> code it must either be port->dev->dev_addr or the rcu
>> macvlan_hash_lookup.
>
> Yes, the corruption is in port->dev->dev_addr. The dev_addr seems to get a
> bogus address value.

Interesting if it is port->dev->dev_addr than count is really out of the
picture.

My blind guess would be that port->dev is getting freed and recycled
before dev_addr gets accessed.  But macvlan_device_event seems to
prevent that.

>> I might just be dense today but I can't possibly see how moving that
>> decrement would solve the crash you have reported below.
>
> In my tests, I have confirmed that with my change, the crash I reported is
> no longer reproducable with our scripts. I have also verified that when I
> pull out your d5cd92448fded change, I can also no longer reproduce the
> issue. So I believe that the crash is related to the above change.
> However, I am not very familier with the code in the macvlan
> driver, so I can not say for sure that the fix I made genuinely solves the
> problem.

It sounds to me like you have a memory stomp and that recompiling the
code winds up changing what gets stomped.

Eric

Re: [PATCH] fix kernel crash in the macvlan driver

[Ani Sinha]

Hi Eric :

I have found the reason for the crash :

On Thu, 7 Jun 2012, Eric W. Biederman wrote:

> >
> > The logic of my change is as follows :
> >
> > As far as I can see, macvlan_newlink() pairs with macvlan_dellink(). If
> > you are incrementing the reference count in newlink(), the corresponding
> > decrement should be, in my opinion in dellink(). If you are derementing
> > the count in uninit(), you are asuming that for every dellink() call,
> > there is a corresponding uninit() call. I am not sure if this assumption
> > is correct. Perhaps you can shed some more lights on this.
>
> Yes.  Look at net/core/dev.c
> dellink calls unregister_netdevice_queue.
> The active part of unregister_netdevice_queue rollback_registered_many
> calls dev->ndo_stop() and then ndo_uninit.

You are correct with respect the current upstream code. However, please
take a look at the commit : 0696c3a8acd3b7c3186dd231d65d97e05a75189f

Before this change was committed, a failure in dev_get_valid_name() would
cause netdev_ops->ndo_uninit() to get called as a part of the clneaup
code. This in turn will mess up (decrement) the port->count values causing
an unbalanced reference cound decrement. This was the reason why the port
was getting freed and a use after free resulted in the crash. I have
verified this was indeed the case using printks. After applying the fix in
0696c3a8acd3b7c3186dd231d65d97e05a75189f  along with your original
ref-count on dev->port fix, I can no longer reproduce the crash.

When moved the counter decrement from uninit() to dellink(), this fixed
the issue since although we had an extra unbalanced uninit() call, it
wasn't modifying the counter values. However ...

>
> We might still be using rcu hash lookups until ndo_close is called and
> so we really don't want to move the decrement before then.

I agree with you on this one. Hence, we should keep the reference counter
modification code where it currently is.


So there was a window when you had applied your patch (port reference
counting) and Peter's fix did not go in. In that period, the upstream
kernel was broken as well. Fortunately, it's all fixed now :)

Cheers,
Ani