From: Eric W. Biederman <hidden> Date: 2012-06-07 19:49:52
Ani Sinha [off-list ref] 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
From: Ani Sinha <hidden> Date: 2012-06-07 20:37:55
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
From: Eric W. Biederman <hidden> Date: 2012-06-07 22:25:18
Ani Sinha [off-list ref] writes:
Hi Eric :
On Thu, 7 Jun 2012, Eric W. Biederman wrote:
quoted
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.
quoted
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.
quoted
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
From: Ani Sinha <hidden> Date: 2012-06-12 01:50:42
Hi Eric :
I have found the reason for the crash :
On Thu, 7 Jun 2012, Eric W. Biederman wrote:
quoted
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