Matthew,
A linux-next oops at bootup in dock_link_device() tells me that you
were not feeling well when you wrote that and dock_unlink_device():
I hope you're feeling better now and can rewrite them soon.
I think my particular problem comes from dock_station_count 0,
so kmalloc() returns NULL, which you don't check for. But those
functions appear to be minor masterpieces of doing things in the
wrong order, not checking allocation, not freeing resource on failure.
I'm not attaching a proposed patch because, on that record, I bet when
you look closer you'll find more wrong that I wouldn't know about.
Let's hope this isn't diverted into an endless discussion of whether
you ought to use kaalloc() or kballoc() or .... me, I'd just like to
boot, and have put #if 0..#endif around everything in those functions.
Hugh
On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> Matthew,
>
> A linux-next oops at bootup in dock_link_device() tells me that you
> were not feeling well when you wrote that and dock_unlink_device():
> I hope you're feeling better now and can rewrite them soon.
Andrew Morton experienced a similar problem. What system are you using?
I didn't encounter this problem with the systems I tested with.
Do you actually have a /sys/devices/platform/dock.?/ directory with a
file 'type' that contains 'dock_station'?
> I think my particular problem comes from dock_station_count 0,
> so kmalloc() returns NULL, which you don't check for. But those
> functions appear to be minor masterpieces of doing things in the
> wrong order, not checking allocation, not freeing resource on failure.
>
> I'm not attaching a proposed patch because, on that record, I bet when
> you look closer you'll find more wrong that I wouldn't know about.
>
> Let's hope this isn't diverted into an endless discussion of whether
> you ought to use kaalloc() or kballoc() or .... me, I'd just like to
> boot, and have put #if 0..#endif around everything in those functions.
Nevertheless, we should check for this error case or make sure we aren't
able to run into it.
Thanks,
Holger
On Fri, 17 Feb 2012, Holger Macht wrote:
> On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> > Matthew,
> >
> > A linux-next oops at bootup in dock_link_device() tells me that you
> > were not feeling well when you wrote that and dock_unlink_device():
> > I hope you're feeling better now and can rewrite them soon.
>
> Andrew Morton experienced a similar problem. What system are you using?
> I didn't encounter this problem with the systems I tested with.
The two systems I got that on were both 4-year-old Core2 Duo systems,
one an HP quad desktop, one a Fujitsu-Siemens laptop.
>
> Do you actually have a /sys/devices/platform/dock.?/ directory with a
> file 'type' that contains 'dock_station'?
I'll have to report back on that this evening, I'm away from them now.
Hugh
On Fr 17. Feb - 14:42:31, Hugh Dickins wrote:
> On Fri, 17 Feb 2012, Holger Macht wrote:
> > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> > > Matthew,
> > >
> > > A linux-next oops at bootup in dock_link_device() tells me that you
> > > were not feeling well when you wrote that and dock_unlink_device():
> > > I hope you're feeling better now and can rewrite them soon.
> >
> > Andrew Morton experienced a similar problem. What system are you using?
> > I didn't encounter this problem with the systems I tested with.
>
> The two systems I got that on were both 4-year-old Core2 Duo systems,
> one an HP quad desktop, one a Fujitsu-Siemens laptop.
Thanks for the information I think this is really independent from the
fact if a laptop, or more precicely if a system with dock station/bay is
used.
>
> >
> > Do you actually have a /sys/devices/platform/dock.?/ directory with a
> > file 'type' that contains 'dock_station'?
>
> I'll have to report back on that this evening, I'm away from them now.
I actually guess that those systems don't have a
/sys/devices/platform/dock.? directory at all, which is fine.
I also think this will fix it, would be great if you could confirm this:
acpi: Bail out when linking devices and there are no dock stations
If dock_station_count is zero, we allocate zero memory and don't check
this at future references. So bail out if there are actually no dock
stations.
Signed-off-by: Holger Macht <[email protected]>
---
drivers/acpi/dock.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index b5e4142..8641912 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle)
int ret, dock = 0;
struct device **devices;
+ if (!dock_station_count)
+ return -ENODEV;
+
devices = kmalloc(dock_station_count * sizeof(struct device *),
GFP_KERNEL);
@@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle)
struct device *dev = acpi_get_physical_device(handle);
struct dock_station *dock_station;
int dock = 0;
- struct device **devices =
- kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ struct device **devices;
+
+ if (!dock_station_count)
+ return -ENODEV;
+
+ devices = kmalloc(dock_station_count * sizeof(struct device *),
+ GFP_KERNEL);
if (!dev)
return NULL;
--
1.7.7
On Sat, 18 Feb 2012, Holger Macht wrote:
> On Fr 17. Feb - 14:42:31, Hugh Dickins wrote:
> > On Fri, 17 Feb 2012, Holger Macht wrote:
> > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> > > > Matthew,
> > > >
> > > > A linux-next oops at bootup in dock_link_device() tells me that you
> > > > were not feeling well when you wrote that and dock_unlink_device():
> > > > I hope you're feeling better now and can rewrite them soon.
> > >
> > > Andrew Morton experienced a similar problem. What system are you using?
> > > I didn't encounter this problem with the systems I tested with.
> >
> > The two systems I got that on were both 4-year-old Core2 Duo systems,
> > one an HP quad desktop, one a Fujitsu-Siemens laptop.
>
> Thanks for the information I think this is really independent from the
> fact if a laptop, or more precicely if a system with dock station/bay is
> used.
>
> >
> > >
> > > Do you actually have a /sys/devices/platform/dock.?/ directory with a
> > > file 'type' that contains 'dock_station'?
> >
> > I'll have to report back on that this evening, I'm away from them now.
>
> I actually guess that those systems don't have a
> /sys/devices/platform/dock.? directory at all, which is fine.
>
> I also think this will fix it, would be great if you could confirm this:
>
> acpi: Bail out when linking devices and there are no dock stations
>
> If dock_station_count is zero, we allocate zero memory and don't check
> this at future references. So bail out if there are actually no dock
> stations.
>
> Signed-off-by: Holger Macht <[email protected]>
Certainly won't fix it as is (well, it shifts the crash over into kfree).
This function is expected to return a pointer, not an error or success code.
I've little doubt that returning NULL rather than -ENODEV there would fix
the boot crash; and if you're in a hurry to fix up booting (understandable)
then I suppose that would do for the moment.
But it won't address the rest of the breakage and leakage in here,
which I suspect will reveal more once Matthew has time to look.
Hugh
> ---
> drivers/acpi/dock.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index b5e4142..8641912 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle)
> int ret, dock = 0;
> struct device **devices;
>
> + if (!dock_station_count)
> + return -ENODEV;
> +
> devices = kmalloc(dock_station_count * sizeof(struct device *),
> GFP_KERNEL);
>
> @@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle)
> struct device *dev = acpi_get_physical_device(handle);
> struct dock_station *dock_station;
> int dock = 0;
> - struct device **devices =
> - kmalloc(dock_station_count * sizeof(struct device *),
> - GFP_KERNEL);
> + struct device **devices;
> +
> + if (!dock_station_count)
> + return -ENODEV;
> +
> + devices = kmalloc(dock_station_count * sizeof(struct device *),
> + GFP_KERNEL);
>
> if (!dev)
> return NULL;
> --
> 1.7.7
On Sat, 18 Feb 2012, Holger Macht wrote:
> On Fr 17. Feb - 14:42:31, Hugh Dickins wrote:
> > On Fri, 17 Feb 2012, Holger Macht wrote:
> > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> > > > Matthew,
> > > >
> > > > A linux-next oops at bootup in dock_link_device() tells me that you
> > > > were not feeling well when you wrote that and dock_unlink_device():
> > > > I hope you're feeling better now and can rewrite them soon.
> > >
> > > Andrew Morton experienced a similar problem. What system are you using?
> > > I didn't encounter this problem with the systems I tested with.
> >
> > The two systems I got that on were both 4-year-old Core2 Duo systems,
> > one an HP quad desktop, one a Fujitsu-Siemens laptop.
>
> Thanks for the information I think this is really independent from the
> fact if a laptop, or more precicely if a system with dock station/bay is
> used.
>
> >
> > >
> > > Do you actually have a /sys/devices/platform/dock.?/ directory with a
> > > file 'type' that contains 'dock_station'?
> >
> > I'll have to report back on that this evening, I'm away from them now.
>
> I actually guess that those systems don't have a
> /sys/devices/platform/dock.? directory at all, which is fine.
You are correct, no /sys/devices/platform/dock.? directory on those.
>
> I also think this will fix it, would be great if you could confirm this:
>
> acpi: Bail out when linking devices and there are no dock stations
>
> If dock_station_count is zero, we allocate zero memory and don't check
> this at future references. So bail out if there are actually no dock
> stations.
>
> Signed-off-by: Holger Macht <[email protected]>
> ---
> drivers/acpi/dock.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index b5e4142..8641912 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle)
> int ret, dock = 0;
> struct device **devices;
>
> + if (!dock_station_count)
> + return -ENODEV;
If I change your -ENODEV to NULL (here and in unlink, though I didn't
check if that gets called), yes, that "fixes" the crash at boot.
But note that in each case you already did an acpi_get_physical_device:
if the existing code was correct in this regard, then you ought to be
doing a put_device(dev) before returning.
Or better reorder it all.
Hugh
> +
> devices = kmalloc(dock_station_count * sizeof(struct device *),
> GFP_KERNEL);
>
> @@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle)
> struct device *dev = acpi_get_physical_device(handle);
> struct dock_station *dock_station;
> int dock = 0;
> - struct device **devices =
> - kmalloc(dock_station_count * sizeof(struct device *),
> - GFP_KERNEL);
> + struct device **devices;
> +
> + if (!dock_station_count)
> + return -ENODEV;
> +
> + devices = kmalloc(dock_station_count * sizeof(struct device *),
> + GFP_KERNEL);
>
> if (!dev)
> return NULL;
> --
> 1.7.7
On Fr 17. Feb - 15:49:02, Hugh Dickins wrote:
> On Sat, 18 Feb 2012, Holger Macht wrote:
> > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote:
> > > On Fri, 17 Feb 2012, Holger Macht wrote:
> > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
> > > > > Matthew,
> > > > >
> > > > > A linux-next oops at bootup in dock_link_device() tells me that you
> > > > > were not feeling well when you wrote that and dock_unlink_device():
> > > > > I hope you're feeling better now and can rewrite them soon.
> > > >
> > > > Andrew Morton experienced a similar problem. What system are you using?
> > > > I didn't encounter this problem with the systems I tested with.
> > >
> > > The two systems I got that on were both 4-year-old Core2 Duo systems,
> > > one an HP quad desktop, one a Fujitsu-Siemens laptop.
> >
> > Thanks for the information I think this is really independent from the
> > fact if a laptop, or more precicely if a system with dock station/bay is
> > used.
> >
> > >
> > > >
> > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a
> > > > file 'type' that contains 'dock_station'?
> > >
> > > I'll have to report back on that this evening, I'm away from them now.
> >
> > I actually guess that those systems don't have a
> > /sys/devices/platform/dock.? directory at all, which is fine.
> >
> > I also think this will fix it, would be great if you could confirm this:
> >
> > acpi: Bail out when linking devices and there are no dock stations
> >
> > If dock_station_count is zero, we allocate zero memory and don't check
> > this at future references. So bail out if there are actually no dock
> > stations.
> >
> > Signed-off-by: Holger Macht <[email protected]>
>
> Certainly won't fix it as is (well, it shifts the crash over into kfree).
> This function is expected to return a pointer, not an error or success
> code.
Oh well, too late for me yesterday...
>
> I've little doubt that returning NULL rather than -ENODEV there would fix
> the boot crash; and if you're in a hurry to fix up booting (understandable)
> then I suppose that would do for the moment.
I really think this will basically do the trick. dock_(un)link_device()
is called by ata_acpi_(un)bind_dock(), which in turn is called by
ata_scsi_scan_host() unconditionally, thus not depending on the presence
of a dock device. And dock_(un)link_device() simply misses the check if
there is a dock/bay device.
So how about that?
acpi: Bail out when linking devices and there are no dock stations
If dock_station_count is zero, we allocate zero memory and don't check
this at future references. So bail out if there are actually no dock
stations.
Signed-off-by: Holger Macht <[email protected]>
---
drivers/acpi/dock.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index b5e4142..0b3072c 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device);
*/
struct device **dock_link_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int ret, dock = 0;
struct device **devices;
+ if (!dock_station_count)
+ return NULL;
+
+ dev = acpi_get_physical_device(handle);
devices = kmalloc(dock_station_count * sizeof(struct device *),
GFP_KERNEL);
@@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
*/
struct device **dock_unlink_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int dock = 0;
- struct device **devices =
- kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ struct device **devices;
+
+ if (!dock_station_count)
+ return NULL;
+
+ dev = acpi_get_physical_device(handle);
+ devices = kmalloc(dock_station_count * sizeof(struct device *),
+ GFP_KERNEL);
if (!dev)
return NULL;
--
1.7.7
On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <[email protected]> wrote:
> On Fr 17. Feb - 15:49:02, Hugh Dickins wrote:
>> On Sat, 18 Feb 2012, Holger Macht wrote:
>> > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote:
>> > > On Fri, 17 Feb 2012, Holger Macht wrote:
>> > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote:
>> > > > > Matthew,
>> > > > >
>> > > > > A linux-next oops at bootup in dock_link_device() tells me that you
>> > > > > were not feeling well when you wrote that and dock_unlink_device():
>> > > > > I hope you're feeling better now and can rewrite them soon.
>> > > >
>> > > > Andrew Morton experienced a similar problem. What system are you using?
>> > > > I didn't encounter this problem with the systems I tested with.
>> > >
>> > > The two systems I got that on were both 4-year-old Core2 Duo systems,
>> > > one an HP quad desktop, one a Fujitsu-Siemens laptop.
>> >
>> > Thanks for the information I think this is really independent from the
>> > fact if a laptop, or more precicely if a system with dock station/bay is
>> > used.
>> >
>> > >
>> > > >
>> > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a
>> > > > file 'type' that contains 'dock_station'?
>> > >
>> > > I'll have to report back on that this evening, I'm away from them now.
>> >
>> > I actually guess that those systems don't have a
>> > /sys/devices/platform/dock.? directory at all, which is fine.
>> >
>> > I also think this will fix it, would be great if you could confirm this:
>> >
>> > acpi: Bail out when linking devices and there are no dock stations
>> >
>> > If dock_station_count is zero, we allocate zero memory and don't check
>> > this at future references. So bail out if there are actually no dock
>> > stations.
>> >
>> > Signed-off-by: Holger Macht <[email protected]>
>>
>> Certainly won't fix it as is (well, it shifts the crash over into kfree).
>> This function is expected to return a pointer, not an error or success
>> code.
>
> Oh well, too late for me yesterday...
>
>>
>> I've little doubt that returning NULL rather than -ENODEV there would fix
>> the boot crash; and if you're in a hurry to fix up booting (understandable)
>> then I suppose that would do for the moment.
>
> I really think this will basically do the trick. dock_(un)link_device()
> is called by ata_acpi_(un)bind_dock(), which in turn is called by
> ata_scsi_scan_host() unconditionally, thus not depending on the presence
> of a dock device. And dock_(un)link_device() simply misses the check if
> there is a dock/bay device.
>
> So how about that?
>
> acpi: Bail out when linking devices and there are no dock stations
>
> If dock_station_count is zero, we allocate zero memory and don't check
> this at future references. So bail out if there are actually no dock
> stations.
>
> Signed-off-by: Holger Macht <[email protected]>
> ---
> drivers/acpi/dock.c | 19 ++++++++++++++-----
> 1 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index b5e4142..0b3072c 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device);
> */
> struct device **dock_link_device(acpi_handle handle)
> {
> - struct device *dev = acpi_get_physical_device(handle);
> + struct device *dev;
> struct dock_station *dock_station;
> int ret, dock = 0;
> struct device **devices;
>
> + if (!dock_station_count)
> + return NULL;
> +
> + dev = acpi_get_physical_device(handle);
> devices = kmalloc(dock_station_count * sizeof(struct device *),
> GFP_KERNEL);
>
> @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
> */
> struct device **dock_unlink_device(acpi_handle handle)
> {
> - struct device *dev = acpi_get_physical_device(handle);
> + struct device *dev;
> struct dock_station *dock_station;
> int dock = 0;
> - struct device **devices =
> - kmalloc(dock_station_count * sizeof(struct device *),
> - GFP_KERNEL);
> + struct device **devices;
> +
> + if (!dock_station_count)
> + return NULL;
> +
> + dev = acpi_get_physical_device(handle);
> + devices = kmalloc(dock_station_count * sizeof(struct device *),
> + GFP_KERNEL);
>
If bail out in this way, another patch looks needed to fix up
mem leakage :-(
> if (!dev)
> return NULL;
> --
> 1.7.7
On Sa 18. Feb - 21:05:18, Hillf Danton wrote:
> On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <[email protected]> wrote:
> > So how about that?
> >
> > acpi: Bail out when linking devices and there are no dock stations
> >
> > If dock_station_count is zero, we allocate zero memory and don't check
> > this at future references. So bail out if there are actually no dock
> > stations.
> >
> > Signed-off-by: Holger Macht <[email protected]>
> > ---
> > ?drivers/acpi/dock.c | ? 19 ++++++++++++++-----
> > ?1 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> > index b5e4142..0b3072c 100644
> > --- a/drivers/acpi/dock.c
> > +++ b/drivers/acpi/dock.c
> > @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device);
> > ?*/
> > ?struct device **dock_link_device(acpi_handle handle)
> > ?{
> > - ? ? ? struct device *dev = acpi_get_physical_device(handle);
> > + ? ? ? struct device *dev;
> > ? ? ? ?struct dock_station *dock_station;
> > ? ? ? ?int ret, dock = 0;
> > ? ? ? ?struct device **devices;
> >
> > + ? ? ? if (!dock_station_count)
> > + ? ? ? ? ? ? ? return NULL;
> > +
> > + ? ? ? dev = acpi_get_physical_device(handle);
> > ? ? ? ?devices = kmalloc(dock_station_count * sizeof(struct device *),
> > ? ? ? ? ? ? ? ? ? ? ? ? ?GFP_KERNEL);
> >
> > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
> > ?*/
> > ?struct device **dock_unlink_device(acpi_handle handle)
> > ?{
> > - ? ? ? struct device *dev = acpi_get_physical_device(handle);
> > + ? ? ? struct device *dev;
> > ? ? ? ?struct dock_station *dock_station;
> > ? ? ? ?int dock = 0;
> > - ? ? ? struct device **devices =
> > - ? ? ? ? ? ? ? kmalloc(dock_station_count * sizeof(struct device *),
> > - ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> > + ? ? ? struct device **devices;
> > +
> > + ? ? ? if (!dock_station_count)
> > + ? ? ? ? ? ? ? return NULL;
> > +
> > + ? ? ? dev = acpi_get_physical_device(handle);
> > + ? ? ? devices = kmalloc(dock_station_count * sizeof(struct device *),
> > + ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> >
>
> If bail out in this way, another patch looks needed to fix up
> mem leakage :-(
Sorry if I'm a little slow...but where is the leakage? The function
doesn't allocate anything before bailing out in the dock_station_count
check. And the rerun value should be freed by the caller. Please point
me in the right direction.
Thanks,
Holger
On Sat, Feb 18, 2012 at 9:26 PM, Holger Macht <[email protected]> wrote:
> On Sa 18. Feb - 21:05:18, Hillf Danton wrote:
>> On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <[email protected]> wrote:
>> > So how about that?
>> >
>> > acpi: Bail out when linking devices and there are no dock stations
>> >
>> > If dock_station_count is zero, we allocate zero memory and don't check
>> > this at future references. So bail out if there are actually no dock
>> > stations.
>> >
>> > Signed-off-by: Holger Macht <[email protected]>
>> > ---
>> > drivers/acpi/dock.c | 19 ++++++++++++++-----
>> > 1 files changed, 14 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
>> > index b5e4142..0b3072c 100644
>> > --- a/drivers/acpi/dock.c
>> > +++ b/drivers/acpi/dock.c
>> > @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device);
>> > */
>> > struct device **dock_link_device(acpi_handle handle)
>> > {
>> > - struct device *dev = acpi_get_physical_device(handle);
>> > + struct device *dev;
>> > struct dock_station *dock_station;
>> > int ret, dock = 0;
>> > struct device **devices;
>> >
>> > + if (!dock_station_count)
>> > + return NULL;
>> > +
>> > + dev = acpi_get_physical_device(handle);
>> > devices = kmalloc(dock_station_count * sizeof(struct device *),
>> > GFP_KERNEL);
>> >
>> > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
>> > */
>> > struct device **dock_unlink_device(acpi_handle handle)
>> > {
>> > - struct device *dev = acpi_get_physical_device(handle);
>> > + struct device *dev;
>> > struct dock_station *dock_station;
>> > int dock = 0;
>> > - struct device **devices =
>> > - kmalloc(dock_station_count * sizeof(struct device *),
>> > - GFP_KERNEL);
>> > + struct device **devices;
>> > +
>> > + if (!dock_station_count)
>> > + return NULL;
>> > +
>> > + dev = acpi_get_physical_device(handle);
>> > + devices = kmalloc(dock_station_count * sizeof(struct device *),
>> > + GFP_KERNEL);
>> >
>>
>> If bail out in this way, another patch looks needed to fix up
>> mem leakage :-(
>
> Sorry if I'm a little slow...but where is the leakage? The function
> doesn't allocate anything before bailing out in the dock_station_count
> check. And the rerun value should be freed by the caller. Please point
> me in the right direction.
>
Hi Holger
Lets check the following snippet from what you posted,
> @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
> */
> struct device **dock_unlink_device(acpi_handle handle)
> {
> - struct device *dev = acpi_get_physical_device(handle);
> + struct device *dev;
> struct dock_station *dock_station;
> int dock = 0;
> - struct device **devices =
> - kmalloc(dock_station_count * sizeof(struct device *),
> - GFP_KERNEL);
> + struct device **devices;
> +
> + if (!dock_station_count)
> + return NULL;
> +
> + dev = acpi_get_physical_device(handle);
> + devices = kmalloc(dock_station_count * sizeof(struct device *),
> + GFP_KERNEL);
>
> if (!dev)
> return NULL; <=== here return without freeing the newly
allocated devices after checking
dock_station_count is not zero
> --
> 1.7.7
>
On Sa 18. Feb - 21:37:18, Hillf Danton wrote:
> On Sat, Feb 18, 2012 at 9:26 PM, Holger Macht <[email protected]> wrote:
> Hi ?Holger
>
> Lets check the following snippet from what you posted,
>
> > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device);
> > ?*/
> > ?struct device **dock_unlink_device(acpi_handle handle)
> > ?{
> > - ? ? ? struct device *dev = acpi_get_physical_device(handle);
> > + ? ? ? struct device *dev;
> > ? ? ? ?struct dock_station *dock_station;
> > ? ? ? ?int dock = 0;
> > - ? ? ? struct device **devices =
> > - ? ? ? ? ? ? ? kmalloc(dock_station_count * sizeof(struct device *),
> > - ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> > + ? ? ? struct device **devices;
> > +
> > + ? ? ? if (!dock_station_count)
> > + ? ? ? ? ? ? ? return NULL;
> > +
> > + ? ? ? dev = acpi_get_physical_device(handle);
> > + ? ? ? devices = kmalloc(dock_station_count * sizeof(struct device *),
> > + ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL);
> >
> > ? ? ? ?if (!dev)
> > ? ? ? ? ? ? ? ?return NULL; <=== here return without freeing the newly
> allocated devices after checking
> dock_station_count is not zero
How about that one?
acpi: Bail out when linking devices and there are no dock stations
If dock_station_count is zero, we allocate zero memory and don't check
this at future references. So bail out if there are actually no dock
stations. Fix some possible memory leaks on the way.
Signed-off-by: Holger Macht <[email protected]>
---
drivers/acpi/dock.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index b5e4142..b2e8730 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -281,14 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device);
*/
struct device **dock_link_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int ret, dock = 0;
struct device **devices;
- devices = kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ if (!dock_station_count)
+ return NULL;
+ dev = acpi_get_physical_device(handle);
if (!dev)
return NULL;
@@ -297,6 +298,9 @@ struct device **dock_link_device(acpi_handle handle)
return NULL;
}
+ devices = kmalloc(dock_station_count * sizeof(struct device *),
+ GFP_KERNEL);
+
list_for_each_entry(dock_station, &dock_stations, sibling) {
if (find_dock_dependent_device(dock_station, handle)) {
ret = sysfs_create_link(&dock_station->dock_device->dev.kobj,
@@ -320,13 +324,15 @@ EXPORT_SYMBOL_GPL(dock_link_device);
*/
struct device **dock_unlink_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int dock = 0;
- struct device **devices =
- kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ struct device **devices;
+
+ if (!dock_station_count)
+ return NULL;
+ dev = acpi_get_physical_device(handle);
if (!dev)
return NULL;
@@ -335,6 +341,9 @@ struct device **dock_unlink_device(acpi_handle handle)
return NULL;
}
+ devices = kmalloc(dock_station_count * sizeof(struct device *),
+ GFP_KERNEL);
+
list_for_each_entry(dock_station, &dock_stations, sibling) {
if (find_dock_dependent_device(dock_station, handle)) {
sysfs_remove_link(&dock_station->dock_device->dev.kobj,
--
1.7.7
On Sat, Feb 18, 2012 at 10:04 PM, Holger Macht <[email protected]> wrote:
>
> How about that one?
>
Thanks for your prompt update :)
Good weekend
Hillf
On Sat, 18 Feb 2012, Holger Macht wrote:
> How about that one?
It's more broken than that. Here's my attempt. It boots on the
systems with dock_station_count 0, and it boots on my laptop with
dock_station_count 2; but I don't actually have any docking station,
so it still doesn't test very much (dock is 0 after the loop).
I have no idea if what goes on in the loop is correct, but it looks
to me as if (as predicted) there's further breakage, that it would
have been writing beyond the end of what it allocated if I did have
a docking station.
Hugh
[PATCH] dock: fix bootup oops and other dock_link breakage
dock_link_device() and dock_unlink_device() should bail out early
to avoid oops on zero-length kmalloc() when dock_station_count is 0.
But isn't there an off-by-one in that kmalloc() length anyway?
An extra NULL appended at the end suggests so.
Rework the ordering with gotos on failure to fix several issues.
And presumably dock_unlink_device() should be presenting the same
interface as dock_link_device(), with NULL returned when none found.
Signed-off-by: Hugh Dickins <[email protected]>
---
drivers/acpi/dock.c | 69 +++++++++++++++++++++++++++++-------------
1 file changed, 49 insertions(+), 20 deletions(-)
--- linux-next/drivers/acpi/dock.c 2012-02-17 08:02:12.280064984 -0800
+++ fixed/drivers/acpi/dock.c 2012-02-18 09:57:54.926244796 -0800
@@ -281,21 +281,25 @@ EXPORT_SYMBOL_GPL(is_dock_device);
*/
struct device **dock_link_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int ret, dock = 0;
struct device **devices;
- devices = kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ if (!dock_station_count)
+ return NULL;
- if (!dev)
+ if (is_dock(handle))
return NULL;
- if (is_dock(handle)) {
- put_device(dev);
+ dev = acpi_get_physical_device(handle);
+ if (!dev)
return NULL;
- }
+
+ devices = kmalloc((dock_station_count + 1) * sizeof(struct device *),
+ GFP_KERNEL);
+ if (!devices)
+ goto put;
list_for_each_entry(dock_station, &dock_stations, sibling) {
if (find_dock_dependent_device(dock_station, handle)) {
@@ -304,13 +308,23 @@ struct device **dock_link_device(acpi_ha
WARN_ON(ret);
devices[dock] = &dock_station->dock_device->dev;
dock++;
+ if (dock == dock_station_count)
+ goto out;
}
}
- if (!dock)
- put_device(dev);
+ if (!dock)
+ goto free;
+out:
+ /* Keep a reference to the device while the link exists */
devices[dock] = NULL;
return devices;
+
+free:
+ kfree(devices);
+put:
+ put_device(dev);
+ return NULL;
}
EXPORT_SYMBOL_GPL(dock_link_device);
@@ -320,20 +334,25 @@ EXPORT_SYMBOL_GPL(dock_link_device);
*/
struct device **dock_unlink_device(acpi_handle handle)
{
- struct device *dev = acpi_get_physical_device(handle);
+ struct device *dev;
struct dock_station *dock_station;
int dock = 0;
- struct device **devices =
- kmalloc(dock_station_count * sizeof(struct device *),
- GFP_KERNEL);
+ struct device **devices;
- if (!dev)
+ if (!dock_station_count)
return NULL;
- if (is_dock(handle)) {
- put_device(dev);
+ if (is_dock(handle))
return NULL;
- }
+
+ dev = acpi_get_physical_device(handle);
+ if (!dev)
+ return NULL;
+
+ devices = kmalloc((dock_station_count + 1) * sizeof(struct device *),
+ GFP_KERNEL);
+ if (!devices)
+ goto put;
list_for_each_entry(dock_station, &dock_stations, sibling) {
if (find_dock_dependent_device(dock_station, handle)) {
@@ -341,15 +360,25 @@ struct device **dock_unlink_device(acpi_
dev_name(dev));
devices[dock] = &dock_station->dock_device->dev;
dock++;
+ if (dock == dock_station_count)
+ goto out;
}
}
- /* An extra reference has been held while the link existed */
- if (dock)
- put_device(dev);
+ if (!dock)
+ goto free;
+out:
+ /* An extra reference has been held while the link existed */
+ put_device(dev);
put_device(dev);
devices[dock] = NULL;
return devices;
+
+free:
+ kfree(devices);
+put:
+ put_device(dev);
+ return NULL;
}
EXPORT_SYMBOL_GPL(dock_unlink_device);
On Sa 18. Feb - 10:46:04, Hugh Dickins wrote:
> On Sat, 18 Feb 2012, Holger Macht wrote:
> > How about that one?
>
> It's more broken than that. Here's my attempt. It boots on the
> systems with dock_station_count 0, and it boots on my laptop with
> dock_station_count 2; but I don't actually have any docking station,
> so it still doesn't test very much (dock is 0 after the loop).
Well, there doesn't have to actually exist a physical dock station (or
bay device) for dock_station_count to be > 0. It just tells that the
ACPI objects are present and thus the system is capable of it.
So does this function actually also break on your laptop and you're
getting the oops there, too?
> I have no idea if what goes on in the loop is correct, but it looks
> to me as if (as predicted) there's further breakage, that it would
> have been writing beyond the end of what it allocated if I did have
> a docking station.
>
> Hugh
>
> [PATCH] dock: fix bootup oops and other dock_link breakage
>
> dock_link_device() and dock_unlink_device() should bail out early
> to avoid oops on zero-length kmalloc() when dock_station_count is 0.
>
> But isn't there an off-by-one in that kmalloc() length anyway?
> An extra NULL appended at the end suggests so.
>
> Rework the ordering with gotos on failure to fix several issues.
>
> And presumably dock_unlink_device() should be presenting the same
> interface as dock_link_device(), with NULL returned when none found.
>
> Signed-off-by: Hugh Dickins <[email protected]>
Fine with me.
Thanks,
Holger
On Sat, 18 Feb 2012, Holger Macht wrote:
> On Sa 18. Feb - 10:46:04, Hugh Dickins wrote:
> > On Sat, 18 Feb 2012, Holger Macht wrote:
> > > How about that one?
> >
> > It's more broken than that. Here's my attempt. It boots on the
> > systems with dock_station_count 0, and it boots on my laptop with
> > dock_station_count 2; but I don't actually have any docking station,
> > so it still doesn't test very much (dock is 0 after the loop).
>
> Well, there doesn't have to actually exist a physical dock station (or
> bay device) for dock_station_count to be > 0. It just tells that the
> ACPI objects are present and thus the system is capable of it.
>
> So does this function actually also break on your laptop and you're
> getting the oops there, too?
It oopsed on the 4-year-old Fujitsu-Siemens laptop whose dock_station_count
was 0. It did not oops on the new ThinkPad laptop whose dock_station_count
is 2, but no docks were found: so the function would only have been leaking
memory on that.
If docks were found, then I suspect it could have been scribbling, but I
cannot actually check if that's true (for all I know, dock_station_count
may be always 1 bigger than the most that that double loop can discover);
but at least the loop is now made safe against scribbling.
Hugh
On Sa 18. Feb - 13:03:34, Hugh Dickins wrote:
> On Sat, 18 Feb 2012, Holger Macht wrote:
> > On Sa 18. Feb - 10:46:04, Hugh Dickins wrote:
> > > On Sat, 18 Feb 2012, Holger Macht wrote:
> > > > How about that one?
> > >
> > > It's more broken than that. Here's my attempt. It boots on the
> > > systems with dock_station_count 0, and it boots on my laptop with
> > > dock_station_count 2; but I don't actually have any docking station,
> > > so it still doesn't test very much (dock is 0 after the loop).
> >
> > Well, there doesn't have to actually exist a physical dock station (or
> > bay device) for dock_station_count to be > 0. It just tells that the
> > ACPI objects are present and thus the system is capable of it.
> >
> > So does this function actually also break on your laptop and you're
> > getting the oops there, too?
>
> It oopsed on the 4-year-old Fujitsu-Siemens laptop whose dock_station_count
> was 0. It did not oops on the new ThinkPad laptop whose dock_station_count
> is 2, but no docks were found: so the function would only have been leaking
> memory on that.
It should actually have successfully linked the dock to the device, if
for instance you had a device in your bay. At least this is working fine
with two Thinkpads I tested with. That's why I didn't encounter this
problem at all before resubmitting the corresponding patch set.
> If docks were found, then I suspect it could have been scribbling, but I
> cannot actually check if that's true (for all I know, dock_station_count
> may be always 1 bigger than the most that that double loop can discover);
> but at least the loop is now made safe against scribbling.
It was actually working fine multiple times, out of pure luck it seems.
Regards,
Holger
On 02/18/2012 02:57 PM, Holger Macht wrote:
> On Sa 18. Feb - 10:46:04, Hugh Dickins wrote:
>> On Sat, 18 Feb 2012, Holger Macht wrote:
>>> How about that one?
>>
>> It's more broken than that. Here's my attempt. It boots on the
>> systems with dock_station_count 0, and it boots on my laptop with
>> dock_station_count 2; but I don't actually have any docking station,
>> so it still doesn't test very much (dock is 0 after the loop).
>
> Well, there doesn't have to actually exist a physical dock station (or
> bay device) for dock_station_count to be> 0. It just tells that the
> ACPI objects are present and thus the system is capable of it.
>
> So does this function actually also break on your laptop and you're
> getting the oops there, too?
>
>> I have no idea if what goes on in the loop is correct, but it looks
>> to me as if (as predicted) there's further breakage, that it would
>> have been writing beyond the end of what it allocated if I did have
>> a docking station.
>>
>> Hugh
>>
>> [PATCH] dock: fix bootup oops and other dock_link breakage
>>
>> dock_link_device() and dock_unlink_device() should bail out early
>> to avoid oops on zero-length kmalloc() when dock_station_count is 0.
>>
>> But isn't there an off-by-one in that kmalloc() length anyway?
>> An extra NULL appended at the end suggests so.
>>
>> Rework the ordering with gotos on failure to fix several issues.
>>
>> And presumably dock_unlink_device() should be presenting the same
>> interface as dock_link_device(), with NULL returned when none found.
>>
>> Signed-off-by: Hugh Dickins<[email protected]>
>
> Fine with me.
So, just to be clear, the preferred patch is Hugh's, and I should drop
your earlier proposed fix found in this thread?
And what about that warning?
Need to fix up linux-next or temporarily drop this patchset from linux-next.
Jeff
On Tue 21. Feb - 17:24:24, Jeff Garzik wrote:
> On 02/18/2012 02:57 PM, Holger Macht wrote:
> >On Sa 18. Feb - 10:46:04, Hugh Dickins wrote:
> >>On Sat, 18 Feb 2012, Holger Macht wrote:
> >>>How about that one?
> >>
> >>It's more broken than that. Here's my attempt. It boots on the
> >>systems with dock_station_count 0, and it boots on my laptop with
> >>dock_station_count 2; but I don't actually have any docking station,
> >>so it still doesn't test very much (dock is 0 after the loop).
> >
> >Well, there doesn't have to actually exist a physical dock station (or
> >bay device) for dock_station_count to be> 0. It just tells that the
> >ACPI objects are present and thus the system is capable of it.
> >
> >So does this function actually also break on your laptop and you're
> >getting the oops there, too?
> >
> >>I have no idea if what goes on in the loop is correct, but it looks
> >>to me as if (as predicted) there's further breakage, that it would
> >>have been writing beyond the end of what it allocated if I did have
> >>a docking station.
> >>
> >>Hugh
> >>
> >>[PATCH] dock: fix bootup oops and other dock_link breakage
> >>
> >>dock_link_device() and dock_unlink_device() should bail out early
> >>to avoid oops on zero-length kmalloc() when dock_station_count is 0.
> >>
> >>But isn't there an off-by-one in that kmalloc() length anyway?
> >>An extra NULL appended at the end suggests so.
> >>
> >>Rework the ordering with gotos on failure to fix several issues.
> >>
> >>And presumably dock_unlink_device() should be presenting the same
> >>interface as dock_link_device(), with NULL returned when none found.
> >>
> >>Signed-off-by: Hugh Dickins<[email protected]>
> >
> >Fine with me.
>
> So, just to be clear, the preferred patch is Hugh's, and I should
> drop your earlier proposed fix found in this thread?
Correct.
> And what about that warning?
You mean the fix for the compile error when compiling with
CONFIG_ACPI_DOCK=n? Here it is again:
[PATCH] acpi: Fix compiler error when setting CONFIG_ACPI_DOCK=n
When compiling with CONFIG_ACPI_DOCK=n,
is_registered_hotplug_dock_device() needs to be defined
Signed-off-by: Holger Macht <[email protected]>
---
include/acpi/acpi_drivers.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 3c4e381..3319574 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -155,6 +155,10 @@ static inline int register_hotplug_dock_device(acpi_handle handle,
static inline void unregister_hotplug_dock_device(acpi_handle handle)
{
}
+static inline int is_registered_hotplug_dock_device(const struct acpi_dock_ops *ops)
+{
+ return 0;
+}
static inline struct device **dock_link_device(acpi_handle handle)
{
return NULL;
--
1.7.7