[PATCH] ls: prevent double open

[PATCH] ls: prevent double open

[Eric Snowberg]

Prevent a double open.  This can cause problems with some ieee1275
devices, causing the system to hang.  The double open can occur
as follows:

grub_ls_list_files (char *dirname, int longlist, int all, int human)
       dev = grub_device_open (device_name);
       dev remains open while:
       grub_normal_print_device_info (device_name);
                dev = grub_device_open (name);

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 grub-core/commands/ls.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 0eaf836..a7318ab 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -201,6 +201,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
 	grub_errno = GRUB_ERR_NONE;
 
+      grub_device_close (dev);
+      dev = NULL;
       grub_normal_print_device_info (device_name);
     }
   else if (fs)
-- 
1.7.1

Re: [PATCH] ls: prevent double open

[Daniel Kiper]

On Mon, Oct 16, 2017 at 01:11:12PM -0700, Eric Snowberg wrote:
> Prevent a double open.  This can cause problems with some ieee1275
> devices, causing the system to hang.  The double open can occur
> as follows:
>
> grub_ls_list_files (char *dirname, int longlist, int all, int human)
>        dev = grub_device_open (device_name);
>        dev remains open while:
>        grub_normal_print_device_info (device_name);
>                 dev = grub_device_open (name);
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

In general LGTM. One nitpick...

> ---
>  grub-core/commands/ls.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> index 0eaf836..a7318ab 100644
> --- a/grub-core/commands/ls.c
> +++ b/grub-core/commands/ls.c
> @@ -201,6 +201,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
>        if (grub_errno == GRUB_ERR_UNKNOWN_FS)
>  	grub_errno = GRUB_ERR_NONE;
>
> +      grub_device_close (dev);
> +      dev = NULL;

I would put a comment before why it is needed.

Thanks,

Daniel

Re: [PATCH] ls: prevent double open

[Eric Snowberg]

> On Oct 17, 2017, at 7:22 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Mon, Oct 16, 2017 at 01:11:12PM -0700, Eric Snowberg wrote:
>> Prevent a double open.  This can cause problems with some ieee1275
>> devices, causing the system to hang.  The double open can occur
>> as follows:
>> 
>> grub_ls_list_files (char *dirname, int longlist, int all, int human)
>>       dev = grub_device_open (device_name);
>>       dev remains open while:
>>       grub_normal_print_device_info (device_name);
>>                dev = grub_device_open (name);
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> In general LGTM. One nitpick...
> 
>> ---
>> grub-core/commands/ls.c |    2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
>> index 0eaf836..a7318ab 100644
>> --- a/grub-core/commands/ls.c
>> +++ b/grub-core/commands/ls.c
>> @@ -201,6 +201,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
>>       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
>> 	grub_errno = GRUB_ERR_NONE;
>> 
>> +      grub_device_close (dev);
>> +      dev = NULL;
> 
> I would put a comment before why it is needed.
> 

Ok, I’ll add a comment and submit a V2.

Re: [PATCH] ls: prevent double open

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Mon, Oct 16, 2017, 22:11 Eric Snowberg <eric.snowberg@oracle.com> wrote:

> Prevent a double open.  This can cause problems with some ieee1275
> devices, causing the system to hang.  The double open can occur
> as follows:
>
Why does it? Underlying firmware device should not be aware of how many
times grub device is open. If it is and it causes bugs, then it's a bug in
device driver

>
> grub_ls_list_files (char *dirname, int longlist, int all, int human)
>        dev = grub_device_open (device_name);
>        dev remains open while:
>        grub_normal_print_device_info (device_name);
>                 dev = grub_device_open (name);
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  grub-core/commands/ls.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> index 0eaf836..a7318ab 100644
> --- a/grub-core/commands/ls.c
> +++ b/grub-core/commands/ls.c
> @@ -201,6 +201,8 @@ grub_ls_list_files (char *dirname, int longlist, int
> all, int human)
>        if (grub_errno == GRUB_ERR_UNKNOWN_FS)
>         grub_errno = GRUB_ERR_NONE;
>
> +      grub_device_close (dev);
> +      dev = NULL;
>        grub_normal_print_device_info (device_name);
>      }
>    else if (fs)
> --
> 1.7.1
>
>

[-- Attachment #2: Type: text/html, Size: 1897 bytes --]

Re: [PATCH] ls: prevent double open

[Eric Snowberg]

> On Oct 18, 2017, at 10:32 AM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:
> 
> 
> 
> On Mon, Oct 16, 2017, 22:11 Eric Snowberg <eric.snowberg@oracle.com> wrote:
> Prevent a double open.  This can cause problems with some ieee1275
> devices, causing the system to hang.  The double open can occur
> as follows:
> Why does it? Underlying firmware device should not be aware of how many times grub device is open. If it is and it causes bugs, then it's a bug in device driver

Which device driver are you referring to?  The one in GRUB or Open Firmware?

Re: [PATCH] ls: prevent double open

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On Wed, Oct 18, 2017, 19:02 Eric Snowberg <eric.snowberg@oracle.com> wrote:

>
> > On Oct 18, 2017, at 10:32 AM, Vladimir 'phcoder' Serbinenko <
> phcoder@gmail.com> wrote:
> >
> >
> >
> > On Mon, Oct 16, 2017, 22:11 Eric Snowberg <eric.snowberg@oracle.com>
> wrote:
> > Prevent a double open.  This can cause problems with some ieee1275
> > devices, causing the system to hang.  The double open can occur
> > as follows:
> > Why does it? Underlying firmware device should not be aware of how many
> times grub device is open. If it is and it causes bugs, then it's a bug in
> device driver
>
> Which device driver are you referring to?  The one in GRUB or Open
> Firmware?
>
In GRUB

>
>
>

[-- Attachment #2: Type: text/html, Size: 1279 bytes --]

Re: [PATCH] ls: prevent double open

[Eric Snowberg]

> On Oct 18, 2017, at 11:03 AM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:
> 
> 
> 
> On Wed, Oct 18, 2017, 19:02 Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
> > On Oct 18, 2017, at 10:32 AM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:
> >
> >
> >
> > On Mon, Oct 16, 2017, 22:11 Eric Snowberg <eric.snowberg@oracle.com> wrote:
> > Prevent a double open.  This can cause problems with some ieee1275
> > devices, causing the system to hang.  The double open can occur
> > as follows:
> > Why does it? Underlying firmware device should not be aware of how many times grub device is open. If it is and it causes bugs, then it's a bug in device driver
> 
> Which device driver are you referring to?  The one in GRUB or Open Firmware?
> In GRUB
> 
> 


I would agree with that.  There are many problems with the ofdisk driver in GRUB. One being the memory corruption issues that prevents new code from being added to it, which is what I believe you are recommending. I submitted a fix for the memory corruption problems years ago, but the patch was ignored.

memory corruption example:

devpath created in grub_ofdisk_open
  it then calls ofdisk_hash_add with devpath
    it then calls ofdisk_hash_add_real with devpath
      ofdisk_hash_add_real saves pointer of devpath
    return
  return
free devpath

dangling pointer/memory corruption with what is stored in ofdisk_hash_add_real, any new memory allocation corrupts devpath

As I recall, just trying to add a grub_printf in the wrong place can corrupt the devpath. I never understood why this bug was too risky to fix for 2.02 or why it still exists today. 

I ended up writing a new driver which we use instead.

https://github.com/esnowberg/grub2-sparc/commit/d802909bc614980dcf6dc2f4484bfa8c2448adf1

This driver would take care of this double open problem making this patch unnecessary. It will also solve many other problems that currently exist in ofdisk. If you would consider this driver in upstream, let me know and I’ll start submitting it.

Re: [PATCH] ls: prevent double open

[John Paul Adrian Glaubitz]

Hi Daniel and Eric!

Is there any progress on these patches?

May I suggest that we continue with the remaining SPARC patches first if
there is still disagreement on the patches that have been posted so far?

I fear that this will fall into oblivion again and we continue to be stuck
with SILO :(.

Please be assured that the Debian sparc64 porters are very happy to help
with any issues that might arise with the patches, be it in the form of
testing or helping to fix potential bugs.

Your work is highly appreciated and much anticipated :).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] ls: prevent double open

[Daniel Kiper]

On Tue, Nov 07, 2017 at 10:17:34PM +0100, John Paul Adrian Glaubitz wrote:
> Hi Daniel and Eric!
>
> Is there any progress on these patches?

Vladimir had some concerns. Sadly he was ill last week and I was going to
ping him this one. Vladimir? If he does not voice his opinion I will apply
this patch by the end of this week.

> May I suggest that we continue with the remaining SPARC patches first if
> there is still disagreement on the patches that have been posted so far?

I am OK with that.

> I fear that this will fall into oblivion again and we continue to be stuck
> with SILO :(.

Unfortunately I am swamped by two internal very pesky bugs and traveling
next week. So, if it is possible to post new patches in the second half
of November I will be very grateful.

> Please be assured that the Debian sparc64 porters are very happy to help
> with any issues that might arise with the patches, be it in the form of
> testing or helping to fix potential bugs.

Great!

> Your work is highly appreciated and much anticipated :).

Thanks!

Daniel

Re: [PATCH] ls: prevent double open

[Daniel Kiper]

On Wed, Oct 18, 2017 at 10:30:37AM -0600, Eric Snowberg wrote:
> > On Oct 17, 2017, at 7:22 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Mon, Oct 16, 2017 at 01:11:12PM -0700, Eric Snowberg wrote:
> >> Prevent a double open.  This can cause problems with some ieee1275
> >> devices, causing the system to hang.  The double open can occur
> >> as follows:
> >>
> >> grub_ls_list_files (char *dirname, int longlist, int all, int human)
> >>       dev = grub_device_open (device_name);
> >>       dev remains open while:
> >>       grub_normal_print_device_info (device_name);
> >>                dev = grub_device_open (name);
> >>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >
> > In general LGTM. One nitpick...
> >
> >> ---
> >> grub-core/commands/ls.c |    2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> >> index 0eaf836..a7318ab 100644
> >> --- a/grub-core/commands/ls.c
> >> +++ b/grub-core/commands/ls.c
> >> @@ -201,6 +201,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
> >>       if (grub_errno == GRUB_ERR_UNKNOWN_FS)
> >> 	grub_errno = GRUB_ERR_NONE;
> >>
> >> +      grub_device_close (dev);
> >> +      dev = NULL;
> >
> > I would put a comment before why it is needed.
> >
>
> Ok, I’ll add a comment and submit a V2.

I was going to apply this today but have not found v2... :-(((
If you post it I will apply it immediately.

Daniel

Re: [PATCH] ls: prevent double open

[John Paul Adrian Glaubitz]

On 11/10/2017 10:35 PM, Daniel Kiper wrote:
>> Ok, I’ll add a comment and submit a V2.
> 
> I was going to apply this today but have not found v2... :-(((
> If you post it I will apply it immediately.

I think Eric didn't send a V2 patch yet. But since the only difference
would be adding an additional explanatory comment, why not apply his
patch and then add the comment in another commit?

Or just apply his patch, add a comment and then commit it?

@Eric: What's your opinion?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913