[PATCH ndctl 0/3] cxl/monitor: coverity and misc other fixes

[PATCH ndctl 0/3] cxl/monitor: coverity and misc other fixes

[Vishal Verma]

Fix a couple of issues reported by Coverity in the cxl-monitor code, and
an async probe wait in the cxl=security.sh test.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
Vishal Verma (3):
      cxl/event_trace: fix a resource leak in cxl_event_to_json()
      cxl/monitor: retain error code in monitor_event()
      test/cxl-security.sh: avoid intermittent failures due to sasync probe

 cxl/event_trace.c | 3 ++-
 cxl/monitor.c     | 3 ++-
 test/security.sh  | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)
---
base-commit: a88bdcfb4202c73aadfee6f83c5502eb5121cbd9
change-id: 20230217-coverity-fixes-e1d3f8bc2cfb

Best regards,
-- 
Vishal Verma <vishal.l.verma@intel.com>

[PATCH ndctl 1/3] cxl/event_trace: fix a resource leak in cxl_event_to_json()

[Vishal Verma]

Static analysis reports that a 'return -ENOMEM' in the above function
bypasses the error unwinding and leaks 'jevent'.

Fix the error handling to use the right goto sequence before returning.

Fixes: 8dedc6cf5e85 ("cxl: add a helper to parse trace events into a json object")
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/event_trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index a973a1f..76dd4e7 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -142,7 +142,8 @@ static int cxl_event_to_json(struct tep_event *event, struct tep_record *record,
 				jobj = num_to_json(data, f->elementsize, f->flags);
 				if (!jobj) {
 					json_object_put(jarray);
-					return -ENOMEM;
+					rc = -ENOMEM;
+					goto err_jevent;
 				}
 				json_object_array_add(jarray, jobj);
 				data += f->elementsize;

-- 
2.39.1

[PATCH ndctl 2/3] cxl/monitor: retain error code in monitor_event()

[Vishal Verma]

Static analysis reports that the error unwinding path in monitor_event()
overwrites 'rc' with the return from cxl_event_tracing_disable(). This
masks the actual error code from either epoll_wait() or
cxl_parse_events() which is the one that should be propagated.

Print a spot error in case there's an error while disabling tracing, but
otherwise retain the rc from the main body of the function.

Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/monitor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 31e6f98..749f472 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx)
 	}
 
 parse_err:
-	rc = cxl_event_tracing_disable(inst);
+	if (cxl_event_tracing_disable(inst) < 0)
+		err(&monitor, "failed to disable tracing\n");
 event_en_err:
 epoll_ctl_err:
 	close(fd);

-- 
2.39.1

[PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Vishal Verma]

This test failed intermittently because the ndctl-list operation right
after a 'modprobe cxl_test' could race the actual nmem devices getting
loaded.

Since CXL device probes are asynchronous, and cxl_acpi might've kicked
off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
everything is loaded.

Add a plain cxl-list right after the modprobe to allow for a flush/wait
cycle.

Cc: Dave Jiang <dave.jiang@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 test/security.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/security.sh b/test/security.sh
index 04f630e..fb04aa6 100755
--- a/test/security.sh
+++ b/test/security.sh
@@ -225,6 +225,7 @@ if [ "$uid" -ne 0 ]; then
 fi
 
 modprobe "$KMOD_TEST"
+cxl list
 setup
 check_prereq "keyctl"
 rc=1

-- 
2.39.1

Re: [PATCH ndctl 1/3] cxl/event_trace: fix a resource leak in cxl_event_to_json()

[Dave Jiang]

On 2/17/23 5:40 PM, Vishal Verma wrote:
> Static analysis reports that a 'return -ENOMEM' in the above function
> bypasses the error unwinding and leaks 'jevent'.
> 
> Fix the error handling to use the right goto sequence before returning.
> 
> Fixes: 8dedc6cf5e85 ("cxl: add a helper to parse trace events into a json object")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   cxl/event_trace.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index a973a1f..76dd4e7 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -142,7 +142,8 @@ static int cxl_event_to_json(struct tep_event *event, struct tep_record *record,
>   				jobj = num_to_json(data, f->elementsize, f->flags);
>   				if (!jobj) {
>   					json_object_put(jarray);
> -					return -ENOMEM;
> +					rc = -ENOMEM;
> +					goto err_jevent;
>   				}
>   				json_object_array_add(jarray, jobj);
>   				data += f->elementsize;
> 

Re: [PATCH ndctl 2/3] cxl/monitor: retain error code in monitor_event()

[Dave Jiang]

On 2/17/23 5:40 PM, Vishal Verma wrote:
> Static analysis reports that the error unwinding path in monitor_event()
> overwrites 'rc' with the return from cxl_event_tracing_disable(). This
> masks the actual error code from either epoll_wait() or
> cxl_parse_events() which is the one that should be propagated.
> 
> Print a spot error in case there's an error while disabling tracing, but
> otherwise retain the rc from the main body of the function.
> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   cxl/monitor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 31e6f98..749f472 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx)
>   	}
>   
>   parse_err:
> -	rc = cxl_event_tracing_disable(inst);
> +	if (cxl_event_tracing_disable(inst) < 0)
> +		err(&monitor, "failed to disable tracing\n");
>   event_en_err:
>   epoll_ctl_err:
>   	close(fd);
> 

Re: [PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Dave Jiang]

On 2/17/23 5:40 PM, Vishal Verma wrote:
> This test failed intermittently because the ndctl-list operation right
> after a 'modprobe cxl_test' could race the actual nmem devices getting
> loaded.
> 
> Since CXL device probes are asynchronous, and cxl_acpi might've kicked
> off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
> everything is loaded.
> 
> Add a plain cxl-list right after the modprobe to allow for a flush/wait
> cycle.
> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   test/security.sh | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/test/security.sh b/test/security.sh
> index 04f630e..fb04aa6 100755
> --- a/test/security.sh
> +++ b/test/security.sh
> @@ -225,6 +225,7 @@ if [ "$uid" -ne 0 ]; then
>   fi
>   
>   modprobe "$KMOD_TEST"
> +cxl list
>   setup
>   check_prereq "keyctl"
>   rc=1
> 

Re: [PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Alison Schofield]

On Fri, Feb 17, 2023 at 05:40:24PM -0700, Vishal Verma wrote:
> This test failed intermittently because the ndctl-list operation right
> after a 'modprobe cxl_test' could race the actual nmem devices getting
> loaded.
> 
> Since CXL device probes are asynchronous, and cxl_acpi might've kicked
> off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
> everything is loaded.
> 
> Add a plain cxl-list right after the modprobe to allow for a flush/wait
> cycle.

Is this the preferred method to 'settle', instead of udevadm settle?

> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  test/security.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/test/security.sh b/test/security.sh
> index 04f630e..fb04aa6 100755
> --- a/test/security.sh
> +++ b/test/security.sh
> @@ -225,6 +225,7 @@ if [ "$uid" -ne 0 ]; then
>  fi
>  
>  modprobe "$KMOD_TEST"
> +cxl list
>  setup
>  check_prereq "keyctl"
>  rc=1
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Dan Williams]

Alison Schofield wrote:
> On Fri, Feb 17, 2023 at 05:40:24PM -0700, Vishal Verma wrote:
> > This test failed intermittently because the ndctl-list operation right
> > after a 'modprobe cxl_test' could race the actual nmem devices getting
> > loaded.
> > 
> > Since CXL device probes are asynchronous, and cxl_acpi might've kicked
> > off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
> > everything is loaded.
> > 
> > Add a plain cxl-list right after the modprobe to allow for a flush/wait
> > cycle.
> 
> Is this the preferred method to 'settle', instead of udevadm settle?

In general, 'udevadm settle' is only the first phase of flushing
subsystem setup work in that it can only flush the udev event queue.
I.e. a device arriving kicks off a KOBJ_ADD event, and once that event
is processed without kicking off more KOBJ_* events the queue is
settled. For CXL this means that the cxl_acpi and cxl_pci modules are
loaded, but the asynchronous work they kick off to probe devices and
rescan the bus is invisible to 'udevadm settle'.

Internally 'cxl list' is performing a 'udevadm settle' and then flushing
the follow on async work.

Re: [PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Verma, Vishal L]

On Tue, 2023-02-21 at 09:22 -0800, Alison Schofield wrote:
> On Fri, Feb 17, 2023 at 05:40:24PM -0700, Vishal Verma wrote:
> > This test failed intermittently because the ndctl-list operation right
> > after a 'modprobe cxl_test' could race the actual nmem devices getting
> > loaded.
> > 
> > Since CXL device probes are asynchronous, and cxl_acpi might've kicked
> > off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
> > everything is loaded.
> > 
> > Add a plain cxl-list right after the modprobe to allow for a flush/wait
> > cycle.
> 
> Is this the preferred method to 'settle', instead of udevadm settle?

Generally, no. Usually cxl tests would use cxl-cli commands, which now
have the necessary waits via cxl_wait_probe(), so even a 'udevadm
settle' shouldn't be needed.

In this case, the first thing we run is ndctl list, which waits for
nvdimm things to 'settle', but we were racing with cxl_test coming up,
which it (ndctl) knows nothing about.

> 
> > 
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  test/security.sh | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/test/security.sh b/test/security.sh
> > index 04f630e..fb04aa6 100755
> > --- a/test/security.sh
> > +++ b/test/security.sh
> > @@ -225,6 +225,7 @@ if [ "$uid" -ne 0 ]; then
> >  fi
> >  
> >  modprobe "$KMOD_TEST"
> > +cxl list
> >  setup
> >  check_prereq "keyctl"
> >  rc=1
> > 
> > -- 
> > 2.39.1
> > 
> > 

Re: [PATCH ndctl 3/3] test/cxl-security.sh: avoid intermittent failures due to sasync probe

[Alison Schofield]

On Tue, Feb 21, 2023 at 10:13:16AM -0800, Vishal Verma wrote:
> On Tue, 2023-02-21 at 09:22 -0800, Alison Schofield wrote:
> > On Fri, Feb 17, 2023 at 05:40:24PM -0700, Vishal Verma wrote:
> > > This test failed intermittently because the ndctl-list operation right
> > > after a 'modprobe cxl_test' could race the actual nmem devices getting
> > > loaded.
> > >
> > > Since CXL device probes are asynchronous, and cxl_acpi might've kicked
> > > off a cxl_bus_rescan(), a cxl_flush() (via cxl_wait_probe()) can ensure
> > > everything is loaded.
> > >
> > > Add a plain cxl-list right after the modprobe to allow for a flush/wait
> > > cycle.
> >
> > Is this the preferred method to 'settle', instead of udevadm settle?
> 
> Generally, no. Usually cxl tests would use cxl-cli commands, which now
> have the necessary waits via cxl_wait_probe(), so even a 'udevadm
> settle' shouldn't be needed.
> 
> In this case, the first thing we run is ndctl list, which waits for
> nvdimm things to 'settle', but we were racing with cxl_test coming up,
> which it (ndctl) knows nothing about.

OK - I'll stop doing the udevadm settle, since what I'm doing is pure
'cxl' and modprobe is always followed by cxl-cli commands. 

> 
> >
> > >
> > > Cc: Dave Jiang <dave.jiang@intel.com>
> > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  test/security.sh | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/test/security.sh b/test/security.sh
> > > index 04f630e..fb04aa6 100755
> > > --- a/test/security.sh
> > > +++ b/test/security.sh
> > > @@ -225,6 +225,7 @@ if [ "$uid" -ne 0 ]; then
> > >  fi
> > >
> > >  modprobe "$KMOD_TEST"
> > > +cxl list
> > >  setup
> > >  check_prereq "keyctl"
> > >  rc=1
> > >
> > > --
> > > 2.39.1
> > >
> > >
> 

Re: [PATCH ndctl 1/3] cxl/event_trace: fix a resource leak in cxl_event_to_json()

[Ira Weiny]

Vishal Verma wrote:
> Static analysis reports that a 'return -ENOMEM' in the above function
> bypasses the error unwinding and leaks 'jevent'.
> 
> Fix the error handling to use the right goto sequence before returning.
> 
> Fixes: 8dedc6cf5e85 ("cxl: add a helper to parse trace events into a json object")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/event_trace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index a973a1f..76dd4e7 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -142,7 +142,8 @@ static int cxl_event_to_json(struct tep_event *event, struct tep_record *record,
>  				jobj = num_to_json(data, f->elementsize, f->flags);
>  				if (!jobj) {
>  					json_object_put(jarray);
> -					return -ENOMEM;
> +					rc = -ENOMEM;
> +					goto err_jevent;
>  				}
>  				json_object_array_add(jarray, jobj);
>  				data += f->elementsize;
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 2/3] cxl/monitor: retain error code in monitor_event()

[Ira Weiny]

Vishal Verma wrote:
> Static analysis reports that the error unwinding path in monitor_event()
> overwrites 'rc' with the return from cxl_event_tracing_disable(). This
> masks the actual error code from either epoll_wait() or
> cxl_parse_events() which is the one that should be propagated.
> 
> Print a spot error in case there's an error while disabling tracing, but
> otherwise retain the rc from the main body of the function.
> 
> Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/monitor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> index 31e6f98..749f472 100644
> --- a/cxl/monitor.c
> +++ b/cxl/monitor.c
> @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx)
>  	}
>  
>  parse_err:
> -	rc = cxl_event_tracing_disable(inst);
> +	if (cxl_event_tracing_disable(inst) < 0)
> +		err(&monitor, "failed to disable tracing\n");

Is this even worth printing?  Perhaps just make
cxl_event_tracing_disable() return void?

Either way:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  event_en_err:
>  epoll_ctl_err:
>  	close(fd);
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH ndctl 2/3] cxl/monitor: retain error code in monitor_event()

[Verma, Vishal L]

On Tue, 2023-02-21 at 17:56 -0800, Ira Weiny wrote:
> Vishal Verma wrote:
> > Static analysis reports that the error unwinding path in monitor_event()
> > overwrites 'rc' with the return from cxl_event_tracing_disable(). This
> > masks the actual error code from either epoll_wait() or
> > cxl_parse_events() which is the one that should be propagated.
> > 
> > Print a spot error in case there's an error while disabling tracing, but
> > otherwise retain the rc from the main body of the function.
> > 
> > Fixes: 299f69f974a6 ("cxl/monitor: add a new monitor command for CXL trace events")
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  cxl/monitor.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cxl/monitor.c b/cxl/monitor.c
> > index 31e6f98..749f472 100644
> > --- a/cxl/monitor.c
> > +++ b/cxl/monitor.c
> > @@ -130,7 +130,8 @@ static int monitor_event(struct cxl_ctx *ctx)
> >         }
> >  
> >  parse_err:
> > -       rc = cxl_event_tracing_disable(inst);
> > +       if (cxl_event_tracing_disable(inst) < 0)
> > +               err(&monitor, "failed to disable tracing\n");
> 
> Is this even worth printing?  Perhaps just make
> cxl_event_tracing_disable() return void?

I thought about it, but the underlying tracefs_trace_off() returns an
int, which is probably why cxl_event_tracing_disable() does too. Having
the print satisfies static analyzers that we're checking the return
value - other than that I agree it doesn't add much.

> 
> Either way:
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks Ira!

> 
> >  event_en_err:
> >  epoll_ctl_err:
> >         close(fd);
> > 
> > -- 
> > 2.39.1
> > 
> > 
> 
>