tools: fix error path of xendevicemodel_open()

[PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Andrew Cooper 3 years, 10 months ago

c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
but failed to fix up the error path.

c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
_open()" fixed up the xencall_open() aspect of the error path (missing the
osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
incorrectly, creating the same pattern proved to be problematic by c/s
30a72f02870 "tools: fix error path of xenhypfs_open()".

Reposition xtl_logger_destroy(), and introduce the missing
osdep_xendevicemodel_close().

Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a compat layer")
Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path in _open()")
Backport: 4.9+
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Paul Durrant <paul@xen.org>

RFC - this is still broken.

Failure to create the logger will still hit the NULL deference, in all of the
stable libs, not just devicemodel.

Also, unless I'd triple checked the history, I was about to reintroduce the
deadlock from c/s 9976f3874d4, because it totally counterintuitive wrong to
expect setup and teardown in opposite orders.
---
 tools/libs/devicemodel/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index db501d9e80..4d4063956d 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -67,9 +67,10 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
     return dmod;
 
 err:
-    xtl_logger_destroy(dmod->logger_tofree);
+    osdep_xendevicemodel_close(dmod);
     xentoolcore__deregister_active_handle(&dmod->tc_ah);
     xencall_close(dmod->xcall);
+    xtl_logger_destroy(dmod->logger_tofree);
     free(dmod);
     return NULL;
 }
-- 
2.11.0

Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Jürgen Groß 3 years, 10 months ago

On 10.06.20 13:40, Andrew Cooper wrote:
> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
> layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
> but failed to fix up the error path.
> 
> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
> _open()" fixed up the xencall_open() aspect of the error path (missing the
> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
> incorrectly, creating the same pattern proved to be problematic by c/s
> 30a72f02870 "tools: fix error path of xenhypfs_open()".
> 
> Reposition xtl_logger_destroy(), and introduce the missing
> osdep_xendevicemodel_close().
> 
> Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a compat layer")
> Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path in _open()")
> Backport: 4.9+
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

RE: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Paul Durrant 3 years, 10 months ago

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 10 June 2020 12:40
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wl@xen.org>; Juergen Gross <jgross@suse.com>; Paul Durrant <paul@xen.org>
> Subject: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
> 
> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
> layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
> but failed to fix up the error path.
> 
> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
> _open()" fixed up the xencall_open() aspect of the error path (missing the
> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
> incorrectly, creating the same pattern proved to be problematic by c/s
> 30a72f02870 "tools: fix error path of xenhypfs_open()".
> 
> Reposition xtl_logger_destroy(), and introduce the missing
> osdep_xendevicemodel_close().
> 
> Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a compat layer")
> Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path in _open()")
> Backport: 4.9+
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Juergen Gross <jgross@suse.com>
> CC: Paul Durrant <paul@xen.org>
> 
> RFC - this is still broken.
> 

I'm slightly confused. Do you want this in 4.14 in this form or are you expecting to update it?

  Paul

> Failure to create the logger will still hit the NULL deference, in all of the
> stable libs, not just devicemodel.
> 
> Also, unless I'd triple checked the history, I was about to reintroduce the
> deadlock from c/s 9976f3874d4, because it totally counterintuitive wrong to
> expect setup and teardown in opposite orders.
> ---
>  tools/libs/devicemodel/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index db501d9e80..4d4063956d 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -67,9 +67,10 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
>      return dmod;
> 
>  err:
> -    xtl_logger_destroy(dmod->logger_tofree);
> +    osdep_xendevicemodel_close(dmod);
>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
>      xencall_close(dmod->xcall);
> +    xtl_logger_destroy(dmod->logger_tofree);
>      free(dmod);
>      return NULL;
>  }
> --
> 2.11.0

Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Andrew Cooper 3 years, 10 months ago

On 12/06/2020 08:22, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>> Sent: 10 June 2020 12:40
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
>> <wl@xen.org>; Juergen Gross <jgross@suse.com>; Paul Durrant <paul@xen.org>
>> Subject: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
>>
>> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
>> layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
>> but failed to fix up the error path.
>>
>> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
>> _open()" fixed up the xencall_open() aspect of the error path (missing the
>> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
>> incorrectly, creating the same pattern proved to be problematic by c/s
>> 30a72f02870 "tools: fix error path of xenhypfs_open()".
>>
>> Reposition xtl_logger_destroy(), and introduce the missing
>> osdep_xendevicemodel_close().
>>
>> Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a compat layer")
>> Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path in _open()")
>> Backport: 4.9+
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Paul Durrant <paul@xen.org>
>>
>> RFC - this is still broken.
>>
> I'm slightly confused. Do you want this in 4.14 in this form or are you expecting to update it?

In this form, it is an improvement over before.

There is still the crash described below which needs some form of
figuring out and fixing.

~Andrew

>
>   Paul
>
>> Failure to create the logger will still hit the NULL deference, in all of the
>> stable libs, not just devicemodel.
>>
>> Also, unless I'd triple checked the history, I was about to reintroduce the
>> deadlock from c/s 9976f3874d4, because it totally counterintuitive wrong to
>> expect setup and teardown in opposite orders.
>> ---
>>  tools/libs/devicemodel/core.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
>> index db501d9e80..4d4063956d 100644
>> --- a/tools/libs/devicemodel/core.c
>> +++ b/tools/libs/devicemodel/core.c
>> @@ -67,9 +67,10 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
>>      return dmod;
>>
>>  err:
>> -    xtl_logger_destroy(dmod->logger_tofree);
>> +    osdep_xendevicemodel_close(dmod);
>>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
>>      xencall_close(dmod->xcall);
>> +    xtl_logger_destroy(dmod->logger_tofree);
>>      free(dmod);
>>      return NULL;
>>  }
>> --
>> 2.11.0
>

RE: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Paul Durrant 3 years, 10 months ago

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 12 June 2020 10:54
> To: paul@xen.org; 'Xen-devel' <xen-devel@lists.xenproject.org>
> Cc: 'Ian Jackson' <Ian.Jackson@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Juergen Gross' <jgross@suse.com>
> Subject: Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
> 
> On 12/06/2020 08:22, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Sent: 10 June 2020 12:40
> >> To: Xen-devel <xen-devel@lists.xenproject.org>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> >> <wl@xen.org>; Juergen Gross <jgross@suse.com>; Paul Durrant <paul@xen.org>
> >> Subject: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
> >>
> >> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
> >> layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
> >> but failed to fix up the error path.
> >>
> >> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
> >> _open()" fixed up the xencall_open() aspect of the error path (missing the
> >> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
> >> incorrectly, creating the same pattern proved to be problematic by c/s
> >> 30a72f02870 "tools: fix error path of xenhypfs_open()".
> >>
> >> Reposition xtl_logger_destroy(), and introduce the missing
> >> osdep_xendevicemodel_close().
> >>
> >> Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a compat layer")
> >> Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path in _open()")
> >> Backport: 4.9+
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Ian Jackson <Ian.Jackson@citrix.com>
> >> CC: Wei Liu <wl@xen.org>
> >> CC: Juergen Gross <jgross@suse.com>
> >> CC: Paul Durrant <paul@xen.org>
> >>
> >> RFC - this is still broken.
> >>
> > I'm slightly confused. Do you want this in 4.14 in this form or are you expecting to update it?
> 
> In this form, it is an improvement over before.
> 
> There is still the crash described below which needs some form of
> figuring out and fixing.
> 

Ok, in which case consider it...

Release-acked-by: Paul Durrant <paul@xen.org>

> ~Andrew
> 
> >
> >   Paul
> >
> >> Failure to create the logger will still hit the NULL deference, in all of the
> >> stable libs, not just devicemodel.
> >>
> >> Also, unless I'd triple checked the history, I was about to reintroduce the
> >> deadlock from c/s 9976f3874d4, because it totally counterintuitive wrong to
> >> expect setup and teardown in opposite orders.
> >> ---
> >>  tools/libs/devicemodel/core.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> >> index db501d9e80..4d4063956d 100644
> >> --- a/tools/libs/devicemodel/core.c
> >> +++ b/tools/libs/devicemodel/core.c
> >> @@ -67,9 +67,10 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
> >>      return dmod;
> >>
> >>  err:
> >> -    xtl_logger_destroy(dmod->logger_tofree);
> >> +    osdep_xendevicemodel_close(dmod);
> >>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
> >>      xencall_close(dmod->xcall);
> >> +    xtl_logger_destroy(dmod->logger_tofree);
> >>      free(dmod);
> >>      return NULL;
> >>  }
> >> --
> >> 2.11.0
> >

Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Ian Jackson 3 years, 10 months ago

Andrew Cooper writes ("Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()"):
> There is still the crash described below which needs some form of
> figuring out and fixing.
...
> >> Failure to create the logger will still hit the NULL deference, in all of the
> >> stable libs, not just devicemodel.

Are you sure ?

I think you mean this sequence of events:

  xencall_open(logger=NULL, open_flags=0)
     xcall->logger = NULL; /* from logger */
     xcall->logger_tofree = NULL;
     if (1) {
       xtl_createlogger_stdiostream => NULL
       /* so */ goto err;
     }

   err:
     xentoolcore__deregister_active_handle(&xcall->tc_ah); /* " */
     osdep_xencall_close(xcall);
     xencall_close(dmod->xcall);
     xtl_logger_destroy(xcall->logger_tofree /* NULL */); // <- crash?
     free(xcall);

But xtl_logger_destroy(NULL) is a safe no-op.

However,

> >> Also, unless I'd triple checked the history, I was about to reintroduce the
> >> deadlock from c/s 9976f3874d4

These comments made me look again at 9976f3874d4 "tools:
xentoolcore_restrict_all: Do deregistration before close".

Just now I wrote:

   I notice that the tail of xendevicemodel_open is now identical to
   xendevicemodel_close.  I think this is expected, and that it would be
   better to combine the two sets of code.  If they hadn't been separate
   then we might have avoided this bug...

But in fact this is not true.  xendevicemodel_close has them in the
right order, but xendevicemodel_open's err block has them in the wrong
order.

Now that I look at xencall, I discover tht xencall_open's err block is
not identical to xencall_close.  xencall close calls
buffer_release_cache and xencall_open's err block does not.  Looking
more closely I think this happens not to be a memory leak because
xcall->buffer* don't contain any malloc'd stuff until they are used.

But I think this is poor practice and another example of teardown code
duplication being a bad idea.

> >> because it totally counterintuitive wrong to
> >> expect setup and teardown in opposite orders.

Are you sure you wrote what you meant ?  To my mind it is usual for
setup and teardown to proceed in precisely the opposite order.

The need to call xentoolcore__deregister_active_handle before closing
the fd is to my mind unusual and counterintuitive.  The reasons are
explained in the commit message for 9976f3874d4cca82.

Does that all make sense ?

Perhaps we should at least delete the wrong err path of
xendevicemodel_open with a call to xendevicemodel_close ?

Ian.

Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

Posted by Ian Jackson 3 years, 10 months ago

Andrew Cooper writes ("[PATCH for-4.14] tools: fix error path of xendevicemodel_open()"):
> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a compat
> layer" introduced calls to both xencall_open() and osdep_xendevicemodel_open()
> but failed to fix up the error path.
> 
> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
> _open()" fixed up the xencall_open() aspect of the error path (missing the
> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
> incorrectly, creating the same pattern proved to be problematic by c/s
> 30a72f02870 "tools: fix error path of xenhypfs_open()".
> 
> Reposition xtl_logger_destroy(), and introduce the missing
> osdep_xendevicemodel_close().

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

I notice that the tail of xendevicemodel_open is now identical to
xendevicemodel_close.  I think this is expected, and that it would be
better to combine the two sets of code.  If they hadn't been separate
then we might have avoided this bug...

Ian.