* [PATCH 1/3] ieee1275: ofdisk dangling pointer
@ 2015-10-26 21:43 Eric Snowberg
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw)
To: grub-devel, eric.snowberg; +Cc: Eric Snowberg
Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
"Don't continue to query block-size if disk doesn't have it.”
a dangling pointer was introduced.
Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
and then used again within the call to grub_ofdisk_get_block_size. This
solves many memory corruption issues we were seeing.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
grub-core/disk/ieee1275/ofdisk.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 331769b..4a5632c 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
op = ofdisk_hash_find (devpath);
if (!op)
op = ofdisk_hash_add (devpath, NULL);
- else
- grub_free (devpath);
if (!op)
- return grub_errno;
+ {
+ grub_free (devpath);
+ return grub_errno;
+ }
disk->id = (unsigned long) op;
disk->data = op->open_path;
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] ieee1275: ofdisk memory leak
2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg
@ 2015-10-26 21:43 ` Eric Snowberg
2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw)
To: grub-devel, eric.snowberg; +Cc: Eric Snowberg
Fix memory leak added within commit:
87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to
query block-size if disk doesn't have it.”
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
grub-core/disk/ieee1275/ofdisk.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 4a5632c..297f058 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
err = grub_ofdisk_get_block_size (devpath, &block_size, op);
if (err)
- return err;
+ {
+ grub_free (devpath);
+ return err;
+ }
if (block_size != 0)
{
for (disk->log_sector_size = 0;
@@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
disk->log_sector_size = 9;
}
+ grub_free (devpath);
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it
2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
@ 2015-10-26 21:43 ` Eric Snowberg
2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-29 14:48 ` Daniel Kiper
3 siblings, 1 reply; 9+ messages in thread
From: Eric Snowberg @ 2015-10-26 21:43 UTC (permalink / raw)
To: grub-devel, eric.snowberg; +Cc: Eric Snowberg
Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
to query block-size if disk doesn't have it.” Disks that returned 0 to the
block-size query, still get queried every time.
Fix logic in grub_ofdisk_get_block_size so the block size is not requested
upon each open since it will not change.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
grub-core/disk/ieee1275/ofdisk.c | 30 +++++++++++++++++++++++-------
1 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
index 297f058..a75ea51 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -35,7 +35,8 @@ struct ofdisk_hash_ent
char *grub_devpath;
int is_boot;
int is_removable;
- int block_size_fails;
+ int block_size_retries;
+ grub_uint32_t block_size;
/* Pointer to shortest available name on nodes representing canonical names,
otherwise NULL. */
const char *shortest;
@@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
disk->log_sector_size = 9;
}
+ if (last_ihandle)
+ grub_ieee1275_close (last_ihandle);
+
+ last_ihandle = 0;
+ last_devpath = NULL;
+ grub_ieee1275_open (devpath, &last_ihandle);
grub_free (devpath);
+
+ if (! last_ihandle)
+ return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
+
return 0;
}
@@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
grub_ieee1275_cell_t size2;
} args_ieee1275;
+ if ((op->block_size_retries >= 2) || (op->block_size > 0))
+ {
+ *block_size = op->block_size;
+ return GRUB_ERR_NONE;
+ }
+
if (last_ihandle)
grub_ieee1275_close (last_ihandle);
@@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
*block_size = 0;
-
- if (op->block_size_fails >= 2)
- return GRUB_ERR_NONE;
+ op->block_size_retries++;
INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2);
args_ieee1275.method = (grub_ieee1275_cell_t) "block-size";
@@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1)
{
grub_dprintf ("disk", "can't get block size: failed call-method\n");
- op->block_size_fails++;
}
else if (args_ieee1275.result)
{
grub_dprintf ("disk", "can't get block size: %lld\n",
(long long) args_ieee1275.result);
- op->block_size_fails++;
}
else if (args_ieee1275.size1
&& !(args_ieee1275.size1 & (args_ieee1275.size1 - 1))
&& args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384)
{
- op->block_size_fails = 0;
+ op->block_size = args_ieee1275.size1;
*block_size = args_ieee1275.size1;
}
+ grub_ieee1275_close (last_ihandle);
+ last_ihandle = 0;
+
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it
2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg
@ 2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-11-10 8:01 ` Andrei Borzenkov
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:02 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]
On 26.10.2015 22:43, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
> to query block-size if disk doesn't have it.” Disks that returned 0 to the
> block-size query, still get queried every time.
>
> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
> upon each open since it will not change.
>
Is it true for removable disks as well? What about someone unplugging
USB stick and plugging-in 4K USB HDD?
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> grub-core/disk/ieee1275/ofdisk.c | 30 +++++++++++++++++++++++-------
> 1 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 297f058..a75ea51 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -35,7 +35,8 @@ struct ofdisk_hash_ent
> char *grub_devpath;
> int is_boot;
> int is_removable;
> - int block_size_fails;
> + int block_size_retries;
> + grub_uint32_t block_size;
> /* Pointer to shortest available name on nodes representing canonical names,
> otherwise NULL. */
> const char *shortest;
> @@ -446,7 +447,17 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
> disk->log_sector_size = 9;
> }
>
> + if (last_ihandle)
> + grub_ieee1275_close (last_ihandle);
> +
> + last_ihandle = 0;
> + last_devpath = NULL;
> + grub_ieee1275_open (devpath, &last_ihandle);
> grub_free (devpath);
> +
> + if (! last_ihandle)
> + return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
> +
> return 0;
> }
>
> @@ -619,6 +630,12 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
> grub_ieee1275_cell_t size2;
> } args_ieee1275;
>
> + if ((op->block_size_retries >= 2) || (op->block_size > 0))
> + {
> + *block_size = op->block_size;
> + return GRUB_ERR_NONE;
> + }
> +
> if (last_ihandle)
> grub_ieee1275_close (last_ihandle);
>
> @@ -630,9 +647,7 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
> return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
>
> *block_size = 0;
> -
> - if (op->block_size_fails >= 2)
> - return GRUB_ERR_NONE;
> + op->block_size_retries++;
>
> INIT_IEEE1275_COMMON (&args_ieee1275.common, "call-method", 2, 2);
> args_ieee1275.method = (grub_ieee1275_cell_t) "block-size";
> @@ -642,21 +657,22 @@ grub_ofdisk_get_block_size (const char *device, grub_uint32_t *block_size,
> if (IEEE1275_CALL_ENTRY_FN (&args_ieee1275) == -1)
> {
> grub_dprintf ("disk", "can't get block size: failed call-method\n");
> - op->block_size_fails++;
> }
> else if (args_ieee1275.result)
> {
> grub_dprintf ("disk", "can't get block size: %lld\n",
> (long long) args_ieee1275.result);
> - op->block_size_fails++;
> }
> else if (args_ieee1275.size1
> && !(args_ieee1275.size1 & (args_ieee1275.size1 - 1))
> && args_ieee1275.size1 >= 512 && args_ieee1275.size1 <= 16384)
> {
> - op->block_size_fails = 0;
> + op->block_size = args_ieee1275.size1;
> *block_size = args_ieee1275.size1;
> }
>
> + grub_ieee1275_close (last_ihandle);
> + last_ihandle = 0;
> +
> return 0;
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer
2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg
@ 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-29 14:48 ` Daniel Kiper
3 siblings, 0 replies; 9+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:03 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]
On 26.10.2015 22:43, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
> "Don't continue to query block-size if disk doesn't have it.”
> a dangling pointer was introduced.
>
> Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
> and then used again within the call to grub_ofdisk_get_block_size. This
> solves many memory corruption issues we were seeing.
>
Committed, thanks
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> grub-core/disk/ieee1275/ofdisk.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 331769b..4a5632c 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -422,10 +422,11 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
> op = ofdisk_hash_find (devpath);
> if (!op)
> op = ofdisk_hash_add (devpath, NULL);
> - else
> - grub_free (devpath);
> if (!op)
> - return grub_errno;
> + {
> + grub_free (devpath);
> + return grub_errno;
> + }
> disk->id = (unsigned long) op;
> disk->data = op->open_path;
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] ieee1275: ofdisk memory leak
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
@ 2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-10-26 22:03 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]
On 26.10.2015 22:43, Eric Snowberg wrote:
> Fix memory leak added within commit:
> 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue to
> query block-size if disk doesn't have it.”
>
Committed, thanks
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> grub-core/disk/ieee1275/ofdisk.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 4a5632c..297f058 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -432,7 +432,10 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>
> err = grub_ofdisk_get_block_size (devpath, &block_size, op);
> if (err)
> - return err;
> + {
> + grub_free (devpath);
> + return err;
> + }
> if (block_size != 0)
> {
> for (disk->log_sector_size = 0;
> @@ -443,6 +446,7 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
> disk->log_sector_size = 9;
> }
>
> + grub_free (devpath);
> return 0;
> }
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] ieee1275: ofdisk dangling pointer
2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg
` (2 preceding siblings ...)
2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko
@ 2015-10-29 14:48 ` Daniel Kiper
3 siblings, 0 replies; 9+ messages in thread
From: Daniel Kiper @ 2015-10-29 14:48 UTC (permalink / raw)
To: The development of GNU GRUB
Cc: john.haxby, jose.marchesi, karl.volz, allen.pais
On Mon, Oct 26, 2015 at 05:43:36PM -0400, Eric Snowberg wrote:
> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 -
> "Don't continue to query block-size if disk doesn't have it.???
> a dangling pointer was introduced.
>
> Fix dangling pointer issue in grub_ofdisk_open where devpath is freed
> and then used again within the call to grub_ofdisk_get_block_size. This
> solves many memory corruption issues we were seeing.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Please CC me too if you repost patches on grub-devel.
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it
2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko
@ 2015-11-10 8:01 ` Andrei Borzenkov
2015-11-10 17:45 ` Eric Snowberg
0 siblings, 1 reply; 9+ messages in thread
From: Andrei Borzenkov @ 2015-11-10 8:01 UTC (permalink / raw)
To: The development of GNU GRUB
On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 26.10.2015 22:43, Eric Snowberg wrote:
>> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
>> to query block-size if disk doesn't have it.” Disks that returned 0 to the
>> block-size query, still get queried every time.
>>
>> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
>> upon each open since it will not change.
>>
> Is it true for removable disks as well? What about someone unplugging
> USB stick and plugging-in 4K USB HDD?
Does OBP alone have hotplug support until OS is running?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it
2015-11-10 8:01 ` Andrei Borzenkov
@ 2015-11-10 17:45 ` Eric Snowberg
0 siblings, 0 replies; 9+ messages in thread
From: Eric Snowberg @ 2015-11-10 17:45 UTC (permalink / raw)
To: The development of GNU GRUB
> On Nov 10, 2015, at 1:01 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>
> On Tue, Oct 27, 2015 at 1:02 AM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
>> On 26.10.2015 22:43, Eric Snowberg wrote:
>>> Within commit: 87ec3b7fa9061f470616ed927fc140e995831c00 - "Don't continue
>>> to query block-size if disk doesn't have it.” Disks that returned 0 to the
>>> block-size query, still get queried every time.
>>>
>>> Fix logic in grub_ofdisk_get_block_size so the block size is not requested
>>> upon each open since it will not change.
>>>
>> Is it true for removable disks as well? What about someone unplugging
>> USB stick and plugging-in 4K USB HDD?
>
> Does OBP alone have hotplug support until OS is running?
AFAIK, SPARC OBP does not have hot plug support.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-11-10 17:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-26 21:43 [PATCH 1/3] ieee1275: ofdisk dangling pointer Eric Snowberg
2015-10-26 21:43 ` [PATCH 2/3] ieee1275: ofdisk memory leak Eric Snowberg
2015-10-26 22:03 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-26 21:43 ` [PATCH 3/3] ieee1275: ofdisk - don't continue to query block-size after we have it Eric Snowberg
2015-10-26 22:02 ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-11-10 8:01 ` Andrei Borzenkov
2015-11-10 17:45 ` Eric Snowberg
2015-10-26 22:03 ` [PATCH 1/3] ieee1275: ofdisk dangling pointer Vladimir 'φ-coder/phcoder' Serbinenko
2015-10-29 14:48 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.