[PATCH] Various fixes - libelf, paging, blktap.

[PATCH] Various fixes - libelf, paging, blktap.

[Konrad Rzeszutek Wilk]

Hey,

I was working on xSplice I enabled various options which ended
up causing compiler issues (except the first one).

The:
 [PATCH v1 1/3] libelf: Preserve log callback after elf_init

was a result of trying to use the libelf in some user-space and
messing up badly but not getting any errors because libelf would
'forget' the log callback.

The other two:
 [PATCH v1 2/3] paging: Fix compile error when DEBUG_TRACE_DUMP is
 [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

Are just fixes for compiler issues.

 tools/blktap2/drivers/block-log.c |  4 ++--
 xen/arch/x86/mm/paging.c          |  4 ++--
 xen/common/libelf/libelf-loader.c | 16 +++++++++++++++-
 3 files changed, 19 insertions(+), 5 deletions(-)

Konrad Rzeszutek Wilk (3):
      libelf: Preserve log callback after elf_init
      paging: Fix compile error when DEBUG_TRACE_DUMP is enabled.
      blktap2: Silence warnings under GCC 5.1.1

[PATCH v1 1/3] libelf: Preserve log callback after elf_init

[Konrad Rzeszutek Wilk]

The elf_init function uses the log callback to report
errors. But it also memsets the whole structure so the
log callback (if set) is wiped out!

This preserves the log callback function along with its
states.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/libelf/libelf-loader.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 6f42bea..32a9724 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -27,14 +27,28 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
 {
     ELF_HANDLE_DECL(elf_shdr) shdr;
     uint64_t i, count, section, offset;
+#ifndef __XEN__
+    elf_log_callback *callback;
+    void *log_callback_data;
+#endif
+    int verbose;
 
     if ( !elf_is_elfbinary(image_input, size) )
     {
         elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__);
         return -1;
     }
-
+#ifndef __XEN__
+    callback = elf->log_callback;
+    log_callback_data = elf->log_caller_data;
+#endif
+    verbose = elf->verbose;
     elf_memset_unchecked(elf, 0, sizeof(*elf));
+#ifndef __XEN__
+    elf->log_callback = callback;
+    elf->log_caller_data = log_callback_data;
+#endif
+    elf->verbose = verbose;
     elf->image_base = image_input;
     elf->size = size;
     elf->ehdr = ELF_MAKE_HANDLE(elf_ehdr, (elf_ptrval)image_input);
-- 
2.4.3

Re: [PATCH v1 1/3] libelf: Preserve log callback after elf_init

[Jan Beulich]

>>> On 03.10.15 at 20:39, <konrad.wilk@oracle.com> wrote:
> The elf_init function uses the log callback to report
> errors. But it also memsets the whole structure so the
> log callback (if set) is wiped out!

Only if you set it before calling elf_init(), which - looking at all current
in-tree users isn't intended.

Jan

Re: [PATCH v1 1/3] libelf: Preserve log callback after elf_init

[Konrad Rzeszutek Wilk]

On Mon, Oct 05, 2015 at 04:36:43AM -0600, Jan Beulich wrote:
> >>> On 03.10.15 at 20:39, <konrad.wilk@oracle.com> wrote:
> > The elf_init function uses the log callback to report
> > errors. But it also memsets the whole structure so the
> > log callback (if set) is wiped out!
> 
> Only if you set it before calling elf_init(), which - looking at all current
> in-tree users isn't intended.

Right? But if you are developing against how else would
you find out that you have issues?

As in, if elf_init can't process the virtual address it tries
to call efi_msg to tell me what I did wrong, but that will always
end up silently (at least on user-space code).

> 
> Jan
> 

Re: [PATCH v1 1/3] libelf: Preserve log callback after elf_init

[Jan Beulich]

>>> On 05.10.15 at 16:23, <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 05, 2015 at 04:36:43AM -0600, Jan Beulich wrote:
>> >>> On 03.10.15 at 20:39, <konrad.wilk@oracle.com> wrote:
>> > The elf_init function uses the log callback to report
>> > errors. But it also memsets the whole structure so the
>> > log callback (if set) is wiped out!
>> 
>> Only if you set it before calling elf_init(), which - looking at all current
>> in-tree users isn't intended.
> 
> Right? But if you are developing against how else would
> you find out that you have issues?
> 
> As in, if elf_init can't process the virtual address it tries
> to call efi_msg to tell me what I did wrong, but that will always
> end up silently (at least on user-space code).

Ah, so you really mean _for_ instead of _after_ in the subject
(and then perhaps also not "preserve")? In that case, however,
I wonder whether the respective information shouldn't be passed
to elf_init() instead of being set up front (e.g. by introducing a
struct elf_log container).

Jan

Re: [PATCH v1 1/3] libelf: Preserve log callback after elf_init

[Konrad Rzeszutek Wilk]

On Mon, Oct 05, 2015 at 08:39:49AM -0600, Jan Beulich wrote:
> >>> On 05.10.15 at 16:23, <konrad.wilk@oracle.com> wrote:
> > On Mon, Oct 05, 2015 at 04:36:43AM -0600, Jan Beulich wrote:
> >> >>> On 03.10.15 at 20:39, <konrad.wilk@oracle.com> wrote:
> >> > The elf_init function uses the log callback to report
> >> > errors. But it also memsets the whole structure so the
> >> > log callback (if set) is wiped out!
> >> 
> >> Only if you set it before calling elf_init(), which - looking at all current
> >> in-tree users isn't intended.
> > 
> > Right? But if you are developing against how else would
> > you find out that you have issues?
> > 
> > As in, if elf_init can't process the virtual address it tries
> > to call efi_msg to tell me what I did wrong, but that will always
> > end up silently (at least on user-space code).
> 
> Ah, so you really mean _for_ instead of _after_ in the subject
> (and then perhaps also not "preserve")? In that case, however,

Argh! Yes!

> I wonder whether the respective information shouldn't be passed
> to elf_init() instead of being set up front (e.g. by introducing a
> struct elf_log container).

<nods>
Let me prep a patch to that effect this week.

> 
> Jan
> 

[PATCH v1 2/3] paging: Fix compile error when DEBUG_TRACE_DUMP is enabled.

[Konrad Rzeszutek Wilk]

And also remove the extra space. 'gmfn' does not exist in this
function anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/mm/paging.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 5becee8..ba1d528 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -331,9 +331,9 @@ void paging_mark_gfn_dirty(struct domain *d, unsigned long pfn)
     unmap_domain_page(l1);
     if ( changed )
     {
-        PAGING_DEBUG(LOGDIRTY, 
+        PAGING_DEBUG(LOGDIRTY,
                      "marked mfn %" PRI_mfn " (pfn=%lx), dom %d\n",
-                     mfn_x(gmfn), pfn, d->domain_id);
+                     mfn_x(mfn), pfn, d->domain_id);
         d->arch.paging.log_dirty.dirty_count++;
     }
 
-- 
2.4.3

Re: [PATCH v1 2/3] paging: Fix compile error when DEBUG_TRACE_DUMP is enabled.

[Andrew Cooper]

On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
> And also remove the extra space. 'gmfn' does not exist in this
> function anymore.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Konrad Rzeszutek Wilk]

I get compile warnings telling me that
   s->connections[i].fd == fd

'i' may be past the array. Adding in an extra condition
on the loop fixes that.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/blktap2/drivers/block-log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/blktap2/drivers/block-log.c b/tools/blktap2/drivers/block-log.c
index 5330cdc..1934227 100644
--- a/tools/blktap2/drivers/block-log.c
+++ b/tools/blktap2/drivers/block-log.c
@@ -359,7 +359,7 @@ static int ctl_close_sock(struct tdlog_state* s, int fd)
 {
   int i;
 
-  for (i = 0; i < s->connected; i++) {
+  for (i = 0; i < s->connected && i < MAX_CONNECTIONS; i++) {
     if (s->connections[i].fd == fd) {
       tapdisk_server_unregister_event(s->connections[i].id);
       close(s->connections[i].fd);
@@ -545,7 +545,7 @@ static inline int ctl_find_connection(struct tdlog_state *s, event_id_t id)
 {
   int i;
 
-  for (i = 0; i < s->connected; i++)
+  for (i = 0; i < s->connected && i < MAX_CONNECTIONS; i++)
     if (s->connections[i].id == id)
       return s->connections[i].fd;
 
-- 
2.4.3

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Andrew Cooper]

On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
> I get compile warnings telling me that
>    s->connections[i].fd == fd
>
> 'i' may be past the array. Adding in an extra condition
> on the loop fixes that.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Furthermore, I can't see any logic which prevents s->connected getting
larger than MAX_CONNECTIONS

> ---
>  tools/blktap2/drivers/block-log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/blktap2/drivers/block-log.c b/tools/blktap2/drivers/block-log.c
> index 5330cdc..1934227 100644
> --- a/tools/blktap2/drivers/block-log.c
> +++ b/tools/blktap2/drivers/block-log.c
> @@ -359,7 +359,7 @@ static int ctl_close_sock(struct tdlog_state* s, int fd)
>  {
>    int i;
>  
> -  for (i = 0; i < s->connected; i++) {
> +  for (i = 0; i < s->connected && i < MAX_CONNECTIONS; i++) {
>      if (s->connections[i].fd == fd) {
>        tapdisk_server_unregister_event(s->connections[i].id);
>        close(s->connections[i].fd);
> @@ -545,7 +545,7 @@ static inline int ctl_find_connection(struct tdlog_state *s, event_id_t id)
>  {
>    int i;
>  
> -  for (i = 0; i < s->connected; i++)
> +  for (i = 0; i < s->connected && i < MAX_CONNECTIONS; i++)
>      if (s->connections[i].id == id)
>        return s->connections[i].fd;
>  

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Jan Beulich]

>>> On 05.10.15 at 10:49, <andrew.cooper3@citrix.com> wrote:
> On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
>> I get compile warnings telling me that
>>    s->connections[i].fd == fd
>>
>> 'i' may be past the array. Adding in an extra condition
>> on the loop fixes that.
>>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Furthermore, I can't see any logic which prevents s->connected getting
> larger than MAX_CONNECTIONS

Iirc Olaf had already suggested such a patch quite some time ago,
and not having seen the point back then I also don't see the point
now: ctl_accept() prevents ->connected from growing beyond 1,
and there's no other place where the value could get incremented.

Also, Konrad, regarding the subject (since this repeats from an
earlier patch of yours) - why do you reference a specific, non-
release version of gcc? Why not simply say 5.x? Because if the
problem is indeed only present in a non-release version, I don't
think we should bother working around such issues.

Jan

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Konrad Rzeszutek Wilk]

On Mon, Oct 05, 2015 at 04:46:55AM -0600, Jan Beulich wrote:
> >>> On 05.10.15 at 10:49, <andrew.cooper3@citrix.com> wrote:
> > On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
> >> I get compile warnings telling me that
> >>    s->connections[i].fd == fd
> >>
> >> 'i' may be past the array. Adding in an extra condition
> >> on the loop fixes that.
> >>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > Furthermore, I can't see any logic which prevents s->connected getting
> > larger than MAX_CONNECTIONS
> 
> Iirc Olaf had already suggested such a patch quite some time ago,
> and not having seen the point back then I also don't see the point
> now: ctl_accept() prevents ->connected from growing beyond 1,
> and there's no other place where the value could get incremented.

Thoughts on what to do about the compiler warnings which make this
compiler errors (since we compile with -Werror) and one can't
compile Xen? A different fix (make s->connected not be an array?)

> 
> Also, Konrad, regarding the subject (since this repeats from an
> earlier patch of yours) - why do you reference a specific, non-
> release version of gcc? Why not simply say 5.x? Because if the
> problem is indeed only present in a non-release version, I don't
> think we should bother working around such issues.

I just ran 'gcc --version' and that is what it spit out. Since
it is part of an official Fedora release I figured it is 'released'
in some way?

> 
> Jan
> 

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Jan Beulich]

>>> On 05.10.15 at 16:26, <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 05, 2015 at 04:46:55AM -0600, Jan Beulich wrote:
>> >>> On 05.10.15 at 10:49, <andrew.cooper3@citrix.com> wrote:
>> > On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
>> >> I get compile warnings telling me that
>> >>    s->connections[i].fd == fd
>> >>
>> >> 'i' may be past the array. Adding in an extra condition
>> >> on the loop fixes that.
>> >>
>> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > 
>> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> > 
>> > Furthermore, I can't see any logic which prevents s->connected getting
>> > larger than MAX_CONNECTIONS
>> 
>> Iirc Olaf had already suggested such a patch quite some time ago,
>> and not having seen the point back then I also don't see the point
>> now: ctl_accept() prevents ->connected from growing beyond 1,
>> and there's no other place where the value could get incremented.
> 
> Thoughts on what to do about the compiler warnings which make this
> compiler errors (since we compile with -Werror) and one can't
> compile Xen? A different fix (make s->connected not be an array?)

To be honest I think this needs to be taken care of at the compiler
side, as I can't see any reason for the warning. The workaround
therefore would be to suppress the warning via -Wno-* until the
compiler side would get fixed. (Of course I'm open to be convinced
otherwise, i.e. this not being a compiler issue.)

>> Also, Konrad, regarding the subject (since this repeats from an
>> earlier patch of yours) - why do you reference a specific, non-
>> release version of gcc? Why not simply say 5.x? Because if the
>> problem is indeed only present in a non-release version, I don't
>> think we should bother working around such issues.
> 
> I just ran 'gcc --version' and that is what it spit out. Since
> it is part of an official Fedora release I figured it is 'released'
> in some way?

Well, if my understanding of gcc's new versioning is correct, only
<x>.<y>.<z> with y > 0 and z == 0 are released versions. 5.1.1
would be an RC for 5.2.0 (which already got released).

Jan

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Konrad Rzeszutek Wilk]

On Mon, Oct 05, 2015 at 08:46:03AM -0600, Jan Beulich wrote:
> >>> On 05.10.15 at 16:26, <konrad.wilk@oracle.com> wrote:
> > On Mon, Oct 05, 2015 at 04:46:55AM -0600, Jan Beulich wrote:
> >> >>> On 05.10.15 at 10:49, <andrew.cooper3@citrix.com> wrote:
> >> > On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
> >> >> I get compile warnings telling me that
> >> >>    s->connections[i].fd == fd
> >> >>
> >> >> 'i' may be past the array. Adding in an extra condition
> >> >> on the loop fixes that.
> >> >>
> >> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> > 
> >> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> > 
> >> > Furthermore, I can't see any logic which prevents s->connected getting
> >> > larger than MAX_CONNECTIONS
> >> 
> >> Iirc Olaf had already suggested such a patch quite some time ago,
> >> and not having seen the point back then I also don't see the point
> >> now: ctl_accept() prevents ->connected from growing beyond 1,
> >> and there's no other place where the value could get incremented.
> > 
> > Thoughts on what to do about the compiler warnings which make this
> > compiler errors (since we compile with -Werror) and one can't
> > compile Xen? A different fix (make s->connected not be an array?)
> 
> To be honest I think this needs to be taken care of at the compiler
> side, as I can't see any reason for the warning. The workaround
> therefore would be to suppress the warning via -Wno-* until the
> compiler side would get fixed. (Of course I'm open to be convinced
> otherwise, i.e. this not being a compiler issue.)

There is an code issue here as well.

Andrew said  - there is no logic that "prevents s->connected getting larger
than MAX_CONNECTIONS." which would resulted in accessing s->connections
outside its array size.

Let me look to see if I can add some s->connected boundaries. Thought
this is going low on my list of priorities.

> 
> >> Also, Konrad, regarding the subject (since this repeats from an
> >> earlier patch of yours) - why do you reference a specific, non-
> >> release version of gcc? Why not simply say 5.x? Because if the
> >> problem is indeed only present in a non-release version, I don't
> >> think we should bother working around such issues.
> > 
> > I just ran 'gcc --version' and that is what it spit out. Since
> > it is part of an official Fedora release I figured it is 'released'
> > in some way?
> 
> Well, if my understanding of gcc's new versioning is correct, only
> <x>.<y>.<z> with y > 0 and z == 0 are released versions. 5.1.1
> would be an RC for 5.2.0 (which already got released).

And indeed: gcc (GCC) 5.1.1 20150618 (Red Hat 5.1.1-4)
while 5.2.0 was out on July 16 (0716). And doing yum upgrade gcc
gives me no new code, hence it seems that Fedora Core 22 is stuck
at an RC for 5.2.0.

> 
> Jan
> 

Re: [PATCH v1 3/3] blktap2: Silence warnings under GCC 5.1.1

[Jan Beulich]

>>> On 05.10.15 at 16:57, <konrad.wilk@oracle.com> wrote:
> On Mon, Oct 05, 2015 at 08:46:03AM -0600, Jan Beulich wrote:
>> >>> On 05.10.15 at 16:26, <konrad.wilk@oracle.com> wrote:
>> > On Mon, Oct 05, 2015 at 04:46:55AM -0600, Jan Beulich wrote:
>> >> >>> On 05.10.15 at 10:49, <andrew.cooper3@citrix.com> wrote:
>> >> > On 03/10/15 19:39, Konrad Rzeszutek Wilk wrote:
>> >> >> I get compile warnings telling me that
>> >> >>    s->connections[i].fd == fd
>> >> >>
>> >> >> 'i' may be past the array. Adding in an extra condition
>> >> >> on the loop fixes that.
>> >> >>
>> >> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> >> > 
>> >> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> > 
>> >> > Furthermore, I can't see any logic which prevents s->connected getting
>> >> > larger than MAX_CONNECTIONS
>> >> 
>> >> Iirc Olaf had already suggested such a patch quite some time ago,
>> >> and not having seen the point back then I also don't see the point
>> >> now: ctl_accept() prevents ->connected from growing beyond 1,
>> >> and there's no other place where the value could get incremented.
>> > 
>> > Thoughts on what to do about the compiler warnings which make this
>> > compiler errors (since we compile with -Werror) and one can't
>> > compile Xen? A different fix (make s->connected not be an array?)
>> 
>> To be honest I think this needs to be taken care of at the compiler
>> side, as I can't see any reason for the warning. The workaround
>> therefore would be to suppress the warning via -Wno-* until the
>> compiler side would get fixed. (Of course I'm open to be convinced
>> otherwise, i.e. this not being a compiler issue.)
> 
> There is an code issue here as well.
> 
> Andrew said  - there is no logic that "prevents s->connected getting larger
> than MAX_CONNECTIONS." which would resulted in accessing s->connections
> outside its array size.

And I replied to that, which I continue to think is not true.

Jan