[RFC PATCH 15/15] Ask some questions in comments

[Eugene Syromiatnikov]

On Mon, Jun 01, 2020 at 01:44:44PM +0200, Ákos Uzonyi wrote:
> ---
>  btree.h              |  8 ++++++++
>  pidns.c              | 20 ++++++++++++++++++++
>  tests/inject-nf.test |  6 ++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/btree.h b/btree.h
> index bd416c87..0aa21afe 100644
> --- a/btree.h
> +++ b/btree.h
> @@ -39,6 +39,14 @@ enum btree_iterate_flags {
>   */
>  struct btree {
>  	uint64_t set_value;         /**< Default set value */
> +
> +	/*
> +	Would it make sense to make this pointer type of void**?
> +	This type is quite confusing anyway, but maybe void** is a bit more clear.
> +
> +	As I see we always cast it to uint64_t* so we use it as a pointer to just
> +	raw data, but for pointer arithmetic we need to point it to a pointer type.

I think it should be simply uint64_t*, I can't recall what I was thinking
about when decided it that way.  btree_get_block() has to be fixed
accordingly (well, triple-asterisk there should be a hint, but, oh, well).

> +	*/
>  	uint64_t **data;
>  	uint8_t item_size_lg;       /**< Item size log2, in bits, 0..6. */
>  	/** Pointer block size log2, in pointers sizes. 8-14, usually. */
> diff --git a/pidns.c b/pidns.c
> index 9433bf47..a3029995 100644
> --- a/pidns.c
> +++ b/pidns.c
> @@ -18,6 +18,10 @@
>  #include "nsfs.h"
>  #include "xmalloc.h"
>  
> +
> +/*
> +Can we check in any efficient way, that these two btree caches are valid?

IIRC, PID NS IDs are not recycled (it has to be verified, though), so we can
rely on that.

> +*/
>  /* key - NS ID, value - parent NS ID. */
>  // struct btree *ns_hierarchy;
>  /*
> @@ -50,6 +54,17 @@ static const struct {
>  
>  struct proc_data {
>  	int proc_pid;
> +
> +	/*
> +	Does it make sense to cache these values below?
> +
> +	As far as i know there is no other way to check this cache validity,
> +	than re-reading these values from /proc. But then we have to read these
> +	values again anyway, so no reason to cache.
> +
> +	The only thing I find useful to cache is proc_pid.
> +	With a cached proc_pid we don't have to read all entries in proc, just one.

In fact, we (probably) can verify that the process is the same by checking
its starttime (it is 64-bit and shouldn't roll over) in /proc/pid/stat.

> +	*/
>  	short ns_count;
>  	short refcount;
>  	uint64_t ns_hierarchy[MAX_NS_DEPTH]; /* from bottom to top of NS hierarchy */
> @@ -78,6 +93,11 @@ pid_to_str(pid_t pid)
>  	return buf;
>  }
>  
> +/*
> +Do we need "last" parameter? Only 0 and our_ns are passed to it, but neither
> +does anything useful, as we can't go higher than our_ns anyway
> +(ioctl fails with EPERM).

I think that the idea was to bail out early when we check pid relations
(for example, if we have 3 PID NS, for strace_NS - PID1_NS - PID2_NS, and we're
looking for PID2 in PID1_NS for whatever reason, then we can finish
early, but I can't remember exactly why I thought it could be useful).

> +*/
>  /**
>   * Returns a list of PID NS IDs for the specified PID.
>   *
> diff --git a/tests/inject-nf.test b/tests/inject-nf.test
> index cadb5adb..9b01345e 100755
> --- a/tests/inject-nf.test
> +++ b/tests/inject-nf.test
> @@ -34,6 +34,12 @@ test_rval()
>  test_rval 0
>  test_rval 1
>  test_rval 0x7fffffff
> +
> +#This test fails (overflow) with these large values,
> +#as we use signed integers in pid translation code
> +#(PID_MAX_LIMIT is 2^22, so that seems reasonable)

The issue is that format has actually changed, it used to be printed
as RVAL_UDECIMAL (kernel_ulong) and it is int now; I would say that the
expected output should be updated in this case (printing -2147483648
seems reasonable to me).  Maybe a separate test that checks that?

> +#
> +#Maybe we could use a different syscall, for example getuid?

Seems reasonable to me.

>  test_rval 0x80000000
>  test_rval 0xfffff000
>  test_rval 0xfffffffe