Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1545847 > unrolled thread

Unregistering extcon providers while they are in use leads to kernel crashes

Back to article view | Back to linux.kernel

Contents

  Unregistering extcon providers while they are in use leads to kernel  crashes Hans de Goede <hdegoede@redhat.com> - 2016-12-21 17:00 +0100
    Re: Unregistering extcon providers while they are in use leads to  kernel crashes Hans de Goede <hdegoede@redhat.com> - 2016-12-21 17:00 +0100
    Re: Unregistering extcon providers while they are in use leads to  kernel crashes Chanwoo Choi <cw00.choi@samsung.com> - 2016-12-27 06:10 +0100

#1545847 — Unregistering extcon providers while they are in use leads to kernel crashes

Hi,

With the recent extcon work I've been doing I noticed that
if I want to rmmod and then insmod say extcon_axp288 I can
do so without problems even if axp288_charger is holding
a reference to the extcon device returned by extcon_get_extcon_dev.

The problem is that extcon_get_extcon_dev simply looks up
the extcon-device in the list of current registered extcon-s
and then returns a pointer to it, without any reference
counting.

The rmmod scenario can be fixed by doing a module_get from
extcon_get_extcon_dev, but that still leaves the same problem
when root manually unbinds the driver through sysfs.

A possible way fix this would be:

1) Make all extcon providers use devm_extcon_dev_allocate and document
using this to allocate an extcon_dev mandatory

2) Add a refcount to struct extcon_dev and introduce extcon_dev_get
and extcon_dev_put helpers which modify the refcount and only free
the memory on the final put (and make the evm_extcon_dev_allocate
cleanup function call extcon_dev_put)

3) On extcon_dev_unregister set a flag in the extcon_dev that it
has been free-ed, make all extcon consumer functions which take
an extcon_dev (extcon_get_state, extcon_register_notifier, etc.)
check this flag and return -ENODEV when the extcon has been unregistered

4) Make extcon_get_extcon_dev call extcon_dev_get on the returned edev
before returning it

 From here on we've fixed the crash, but we now leak the extcon_dev
when the consumer gets unbound.

5) Add a devm_extcon_get_extcon_dev which calls extcon_dev_put as the devm
cleanup function

6) Convert all extcon consumers to use devm_extcon_get_extcon_dev

Regards,

Hans

[toc] | [next] | [standalone]

#1545851 — Re: Unregistering extcon providers while they are in use leads to kernel crashes

Hi,

On 21-12-16 16:54, Hans de Goede wrote:
> Hi,
>
> With the recent extcon work I've been doing I noticed that
> if I want to rmmod and then insmod say extcon_axp288 I can
> do so without problems even if axp288_charger is holding
> a reference to the extcon device returned by extcon_get_extcon_dev.
>
> The problem is that extcon_get_extcon_dev simply looks up
> the extcon-device in the list of current registered extcon-s
> and then returns a pointer to it, without any reference
> counting.
>
> The rmmod scenario can be fixed by doing a module_get from
> extcon_get_extcon_dev, but that still leaves the same problem
> when root manually unbinds the driver through sysfs.
>
> A possible way fix this would be:
>
> 1) Make all extcon providers use devm_extcon_dev_allocate and document
> using this to allocate an extcon_dev mandatory
>
> 2) Add a refcount to struct extcon_dev and introduce extcon_dev_get
> and extcon_dev_put helpers which modify the refcount and only free
> the memory on the final put (and make the evm_extcon_dev_allocate
> cleanup function call extcon_dev_put)
>
> 3) On extcon_dev_unregister set a flag in the extcon_dev that it
> has been free-ed, make all extcon consumer functions which take
> an extcon_dev (extcon_get_state, extcon_register_notifier, etc.)
> check this flag and return -ENODEV when the extcon has been unregistered
>
> 4) Make extcon_get_extcon_dev call extcon_dev_get on the returned edev
> before returning it
>
> From here on we've fixed the crash, but we now leak the extcon_dev
> when the consumer gets unbound.
>
> 5) Add a devm_extcon_get_extcon_dev which calls extcon_dev_put as the devm
> cleanup function
>
> 6) Convert all extcon consumers to use devm_extcon_get_extcon_dev

p.s.

In case it was not clear, I noticed this, but I don't have the time to
fix it, which is why I wrote the above plan with the hope that someone
can use it as a base to actual fix this.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1547494 — Re: Unregistering extcon providers while they are in use leads to kernel crashes

Hi Hans,

Thanks for your report.
I'll check this problem and try to resolve it.

On 2016년 12월 22일 00:54, Hans de Goede wrote:
> Hi,
> 
> With the recent extcon work I've been doing I noticed that
> if I want to rmmod and then insmod say extcon_axp288 I can
> do so without problems even if axp288_charger is holding
> a reference to the extcon device returned by extcon_get_extcon_dev.
> 
> The problem is that extcon_get_extcon_dev simply looks up
> the extcon-device in the list of current registered extcon-s
> and then returns a pointer to it, without any reference
> counting.
> 
> The rmmod scenario can be fixed by doing a module_get from
> extcon_get_extcon_dev, but that still leaves the same problem
> when root manually unbinds the driver through sysfs.
> 
> A possible way fix this would be:
> 
> 1) Make all extcon providers use devm_extcon_dev_allocate and document
> using this to allocate an extcon_dev mandatory
> 
> 2) Add a refcount to struct extcon_dev and introduce extcon_dev_get
> and extcon_dev_put helpers which modify the refcount and only free
> the memory on the final put (and make the evm_extcon_dev_allocate
> cleanup function call extcon_dev_put)
> 
> 3) On extcon_dev_unregister set a flag in the extcon_dev that it
> has been free-ed, make all extcon consumer functions which take
> an extcon_dev (extcon_get_state, extcon_register_notifier, etc.)
> check this flag and return -ENODEV when the extcon has been unregistered
> 
> 4) Make extcon_get_extcon_dev call extcon_dev_get on the returned edev
> before returning it
> 
> From here on we've fixed the crash, but we now leak the extcon_dev
> when the consumer gets unbound.
> 
> 5) Add a devm_extcon_get_extcon_dev which calls extcon_dev_put as the devm
> cleanup function
> 
> 6) Convert all extcon consumers to use devm_extcon_get_extcon_dev
> 
> Regards,
> 
> Hans
> 
> 
> 

-- 
Regards,
Chanwoo Choi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web