Re: [PATCH 31/38] Fix dt_pebs_init() call

[Kris Van Hees <kris.van.hees@oracle.com>]

This patch goes against the intent of the code, as far as I can see.  The
point of performing the size check prior to increasing the buffer size was
to enforce the limit on the size that was explicit set by the user.  Then,
if that size was sufficient, then DTrace is at liberty to increase that
size to a whole multiple of the page size.

But if I were to insist that a buffer can only be 24 bytes, and 24 bytes are
not enough to store a single trace record, DTrace should report an error
rather than increasing my bufsize setting on its own.

On Thu, Jun 27, 2024 at 01:38:57AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The function had a few issues, mainly with its comments,
> ranging from typos to incorrect descriptions of behavior.
> 
> The function has only one caller.  The enforcement of a
> size minimum was problematic in a number of respects:
> - it was missing 4 bytes
> - it was enforcing the minimum before the size was
>     increased by the caller
> - it was checking dt_pebs_init()==-1,
>     missing errors with other negative return values
> 
> So change the function to accept a minimum and change its
> return values.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_peb.c                          | 27 ++++++++++-----------
>  libdtrace/dt_peb.h                          |  2 +-
>  libdtrace/dt_work.c                         | 15 +++++++-----
>  test/unittest/options/err.b-too-low.d       | 22 +++++++++--------
>  test/unittest/options/err.bufsize-too-low.d | 22 +++++++++--------
>  test/unittest/options/tst.b.d               | 22 +++++++++--------
>  test/unittest/options/tst.bufsize.d         | 22 +++++++++--------
>  7 files changed, 71 insertions(+), 61 deletions(-)
> 
> diff --git a/libdtrace/dt_peb.c b/libdtrace/dt_peb.c
> index 5268f089..4983ba91 100644
> --- a/libdtrace/dt_peb.c
> +++ b/libdtrace/dt_peb.c
> @@ -136,18 +136,14 @@ dt_pebs_exit(dtrace_hdl_t *dtp)
>  }
>  
>  /*
> - * Initialize the perf event buffers (one per online CPU).  Each buffer will
> - * the given number of pages (i.e. the total size of each buffer will be
> - * num_pages * getpagesize()).  The  allocated memory for each buffer is mmap'd
> - * so the kernel can write to it, and its representative file descriptor is
> - * recorded in the 'buffers' BPF map so that BPF code knows where to write
> - * trace data for a specific CPU.
> + * Initialize the perf event buffers, one per online CPU.  Each buffer will
> + * have num_pages * getpagesize()).  The dt_peb_open() call mmaps the allocated
> + * memory so the kernel can write to it.  The file descriptor is recorded in
> + * the 'buffers' BPF map and added to the event polling file descriptor.
>   *
> - * An event polling file descriptor is created as well, and it is configured to
> - * monitor all perf event buffers at once.  This file descriptor is returned
> - * upon success..  Failure is indicated with a -1 return value.
> + * The return value indicates success (0) or failure (-1).
>   */
> -int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
> +int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize, size_t bufsizemin)
>  {
>  	int		i;
>  	int		mapfd;
> @@ -166,12 +162,15 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
>  		fprintf(stderr, "bufsize increased to %lu\n",
>  			num_pages * getpagesize());
>  
> +	if (num_pages * getpagesize() < bufsizemin)
> +		return dt_set_errno(dtp, EDT_BUFTOOSMALL);
> +
>  	/*
>  	 * Determine the fd for the 'buffers' BPF map.
>  	 */
>  	idp = dt_dlib_get_map(dtp, "buffers");
>  	if (idp == NULL || idp->di_id == DT_IDENT_UNDEF)
> -		return -ENOENT;
> +		return dt_set_errno(dtp, EDT_NOMEM);    // FIXME we used to do something more akin to ENOENT
>  
>  	mapfd = idp->di_id;
>  
> @@ -180,7 +179,7 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
>  	 */
>  	dtp->dt_pebset = dt_zalloc(dtp, sizeof(dt_pebset_t));
>  	if (dtp->dt_pebset == NULL)
> -		return -ENOMEM;
> +		return dt_set_errno(dtp, EDT_NOMEM);
>  
>  	/*
>  	 * Allocate the per-CPU perf event buffers.
> @@ -189,7 +188,7 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
>  			 sizeof(struct dt_peb));
>  	if (pebs == NULL) {
>  		dt_free(dtp, dtp->dt_pebset);
> -		return -ENOMEM;
> +		return dt_set_errno(dtp, EDT_NOMEM);
>  	}
>  
>  	dtp->dt_pebset->pebs = pebs;
> @@ -241,5 +240,5 @@ int dt_pebs_init(dtrace_hdl_t *dtp, size_t bufsize)
>  fail:
>  	dt_pebs_exit(dtp);
>  
> -	return -1;
> +	return dt_set_errno(dtp, EDT_NOMEM);  // FIXME need something better
>  }
> diff --git a/libdtrace/dt_peb.h b/libdtrace/dt_peb.h
> index e0f408f2..9ad23252 100644
> --- a/libdtrace/dt_peb.h
> +++ b/libdtrace/dt_peb.h
> @@ -43,7 +43,7 @@ typedef struct dt_pebset {
>  } dt_pebset_t;
>  
>  extern void dt_pebs_exit(dtrace_hdl_t *);
> -extern int dt_pebs_init(dtrace_hdl_t *, size_t);
> +extern int dt_pebs_init(dtrace_hdl_t *, size_t, size_t);
>  
>  #ifdef	__cplusplus
>  }
> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> index 69a86358..7a0eb1da 100644
> --- a/libdtrace/dt_work.c
> +++ b/libdtrace/dt_work.c
> @@ -267,18 +267,21 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
>  		return dt_set_errno(dtp, errno);
>  
>  	/*
> -	 * We need enough space for the pref_event_header, a 32-bit size, a
> +	 * We need enough space for the perf_event_header, a 32-bit size, a
>  	 * 4-byte gap, and the largest trace data record we may be writing to
>  	 * the buffer.  In other words, the buffer needs to be large enough to
>  	 * hold at least one perf-encapsulated trace data record.
> +	 *
> +	 * While dt_pebs_init() rounds the requested size up, size==0 is a
> +	 * special case.
>  	 */
>  	dtrace_getopt(dtp, "bufsize", &size);
> -	if (size == 0 ||
> -	    size < sizeof(struct perf_event_header) + sizeof(uint32_t) +
> -		   dtp->dt_maxreclen)
> +	if (size == 0)
>  		return dt_set_errno(dtp, EDT_BUFTOOSMALL);
> -	if (dt_pebs_init(dtp, size) == -1)
> -		return dt_set_errno(dtp, EDT_NOMEM);
> +	if (dt_pebs_init(dtp, size,
> +			 sizeof(struct perf_event_header) + sizeof(uint32_t)
> +			 + 4 + dtp->dt_maxreclen) == -1)
> +		return -1;
>  
>  	/*
>  	 * We must initialize the aggregation consumer handling before we
> diff --git a/test/unittest/options/err.b-too-low.d b/test/unittest/options/err.b-too-low.d
> index bb77e37c..f62155dd 100644
> --- a/test/unittest/options/err.b-too-low.d
> +++ b/test/unittest/options/err.b-too-low.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> @@ -12,19 +12,21 @@
>   */
>  
>  /*
> - * We use a buffer size of 59 because that should be just too small to hold the
> - * trace records generated in this script:
> - *	- perf_event_header (40 bytes)
> - *	- size (4 bytes)
> - *	- gap (4 bytes)
> - *	- EPID (4 bytes)
> - *	- tag (4 bytes)
> - *	- exit value (4 bytes)
> + * We need over 4k bytes for the 4 string trace records generated in this script
> + * plus some meta data.
> + * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
> + * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
>   */
> -/* @@runtest-opts: -b59 */
> +/* @@runtest-opts: -b4096 */
> +
> +#pragma D option strsize=1024
>  
>  BEGIN
>  {
> +	trace("abc");
> +	trace("def");
> +	trace("ghi");
> +	trace("jkl");
>  	exit(0);
>  }
>  
> diff --git a/test/unittest/options/err.bufsize-too-low.d b/test/unittest/options/err.bufsize-too-low.d
> index bbbdb5c5..25efdf72 100644
> --- a/test/unittest/options/err.bufsize-too-low.d
> +++ b/test/unittest/options/err.bufsize-too-low.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> @@ -12,19 +12,21 @@
>   */
>  
>  /*
> - * We use a buffer size of 59 because that should be just too small to hold the
> - * trace records generated in this script:
> - *	- perf_event_header (40 bytes)
> - *	- size (4 bytes)
> - *	- gap (4 bytes)
> - *	- EPID (4 bytes)
> - *	- tag (4 bytes)
> - *	- exit value (4 bytes)
> + * We need over 4k bytes for the 4 string trace records generated in this script
> + * plus some meta data.
> + * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
> + * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
>   */
> -/* @@runtest-opts: -xbufsize=59 */
> +/* @@runtest-opts: -xbufsize=4096 */
> +
> +#pragma D option strsize=1024
>  
>  BEGIN
>  {
> +	trace("abc");
> +	trace("def");
> +	trace("ghi");
> +	trace("jkl");
>  	exit(0);
>  }
>  
> diff --git a/test/unittest/options/tst.b.d b/test/unittest/options/tst.b.d
> index 57fa030d..3bf08edc 100644
> --- a/test/unittest/options/tst.b.d
> +++ b/test/unittest/options/tst.b.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> @@ -12,19 +12,21 @@
>   */
>  
>  /*
> - * We use a buffer size of 60 because that should be the exact size necessary
> - * to hold the trace records generated in this script:
> - *	- perf_event_header (40 bytes)
> - *	- size (4 bytes)
> - *	- gap (4 bytes)
> - *	- EPID (4 bytes)
> - *	- tag (4 bytes)
> - *	- exit value (4 bytes)
> + * We need over 4k bytes for the 4 string trace records generated in this script
> + * plus some meta data.
> + * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
> + * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
>   */
> -/* @@runtest-opts: -b60 */
> +/* @@runtest-opts: -b4097 */
> +
> +#pragma D option strsize=1024
>  
>  BEGIN
>  {
> +	trace("abc");
> +	trace("def");
> +	trace("ghi");
> +	trace("jkl");
>  	exit(0);
>  }
>  
> diff --git a/test/unittest/options/tst.bufsize.d b/test/unittest/options/tst.bufsize.d
> index 96b0f1b8..23af81aa 100644
> --- a/test/unittest/options/tst.bufsize.d
> +++ b/test/unittest/options/tst.bufsize.d
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> @@ -12,19 +12,21 @@
>   */
>  
>  /*
> - * We use a buffer size of 60 because that should be the exact size necessary
> - * to hold the trace records generated in this script:
> - *	- perf_event_header (40 bytes)
> - *	- size (4 bytes)
> - *	- gap (4 bytes)
> - *	- EPID (4 bytes)
> - *	- tag (4 bytes)
> - *	- exit value (4 bytes)
> + * We need over 4k bytes for the 4 string trace records generated in this script
> + * plus some meta data.
> + * A bufsize of 1 to 4096 bytes is rounded up to 4096 and is insufficient.
> + * A bufsize of 4097 to 8192 bytes is rounded up to 8192 and is sufficient.
>   */
> -/* @@runtest-opts: -xbufsize=60 */
> +/* @@runtest-opts: -xbufsize=4097 */
> +
> +#pragma D option strsize=1024
>  
>  BEGIN
>  {
> +	trace("abc");
> +	trace("def");
> +	trace("ghi");
> +	trace("jkl");
>  	exit(0);
>  }
>  
> -- 
> 2.18.4
>