netdev - Re: [PATCH v6 bpf-next 2/6] bpf: move to generic BTF show support, apply it to seq files/strings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200925003323.u2s2vyyqq2uhtij7@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 24 Sep 2020 17:33:23 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andriin@...com, yhs@...com,
        linux@...musvillemoes.dk, andriy.shevchenko@...ux.intel.com,
        pmladek@...e.com, kafai@...com, songliubraving@...com,
        john.fastabend@...il.com, kpsingh@...omium.org, shuah@...nel.org,
        rdna@...com, scott.branden@...adcom.com, quentin@...valent.com,
        cneirabustos@...il.com, jakub@...udflare.com, mingo@...hat.com,
        rostedt@...dmis.org, bpf@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        acme@...nel.org
Subject: Re: [PATCH v6 bpf-next 2/6] bpf: move to generic BTF show support,
 apply it to seq files/strings

On Wed, Sep 23, 2020 at 06:46:24PM +0100, Alan Maguire wrote:
>  
> +/* Chunk size we use in safe copy of data to be shown. */
> +#define BTF_SHOW_OBJ_SAFE_SIZE		256

sizeof(struct btf_show) == 472
It's allocated on stack and called from bpf prog.
It's a leaf function, but it still worries me a bit.
I've trimmed it down to 32 and everything seems to be printing fine.
There will be more calls to copy_from_kernel_nofault(), but so what?
Is there a downside to make it that small?

Similarly state.name is 128 bytes. May be use 80 there?
I think that should be plenty still.

> + * Another problem is we want to ensure the data for display is safe to
> + * access.  To support this, the "struct obj" is used to track the data

'struct obj' doesn't exist. It's an anon field 'struct {} obj;' inside btf_show
that you're referring to, right?
Would be good to fix this comment.

> +struct btf_show {
> +	u64 flags;
> +	void *target;	/* target of show operation (seq file, buffer) */
> +	void (*showfn)(struct btf_show *show, const char *fmt, ...);

buildbot complained that this field needs to be annotated.

> +#define btf_show(show, ...)						      \
> +	do {								      \
> +		if (!show->state.depth_check)				      \
> +			show->showfn(show, __VA_ARGS__);		      \
> +	} while (0)

Does it have to be a macro? What are you gaining from macro
instead of vararg function?

> +static inline const char *__btf_show_indent(struct btf_show *show)

please remove all 'inline' from .c file.
There is no need to give such hints to the compiler.

> +#define btf_show_indent(show)						       \
> +	((show->flags & BTF_SHOW_COMPACT) ? "" : __btf_show_indent(show))
> +
> +#define btf_show_newline(show)						       \
> +	((show->flags & BTF_SHOW_COMPACT) ? "" : "\n")
> +
> +#define btf_show_delim(show)						       \
> +	(show->state.depth == 0 ? "" :					       \
> +	 ((show->flags & BTF_SHOW_COMPACT) && show->state.type &&	       \
> +	  BTF_INFO_KIND(show->state.type->info) == BTF_KIND_UNION) ? "|" : ",")
> +
> +#define btf_show_type_value(show, fmt, value)				       \
> +	do {								       \
> +		if ((value) != 0 || (show->flags & BTF_SHOW_ZERO) ||	       \
> +		    show->state.depth == 0) {				       \
> +			btf_show(show, "%s%s" fmt "%s%s",		       \
> +				 btf_show_indent(show),			       \
> +				 btf_show_name(show),			       \
> +				 value, btf_show_delim(show),		       \
> +				 btf_show_newline(show));		       \
> +			if (show->state.depth > show->state.depth_to_show)     \
> +				show->state.depth_to_show = show->state.depth; \
> +		}							       \
> +	} while (0)
> +
> +#define btf_show_type_values(show, fmt, ...)				       \
> +	do {								       \
> +		btf_show(show, "%s%s" fmt "%s%s", btf_show_indent(show),       \
> +			 btf_show_name(show),				       \
> +			 __VA_ARGS__, btf_show_delim(show),		       \
> +			 btf_show_newline(show));			       \
> +		if (show->state.depth > show->state.depth_to_show)	       \
> +			show->state.depth_to_show = show->state.depth;	       \
> +	} while (0)
> +
> +/* How much is left to copy to safe buffer after @data? */
> +#define btf_show_obj_size_left(show, data)				       \
> +	(show->obj.head + show->obj.size - data)
> +
> +/* Is object pointed to by @data of @size already copied to our safe buffer? */
> +#define btf_show_obj_is_safe(show, data, size)				       \
> +	(data >= show->obj.data &&					       \
> +	 (data + size) < (show->obj.data + BTF_SHOW_OBJ_SAFE_SIZE))
> +
> +/*
> + * If object pointed to by @data of @size falls within our safe buffer, return
> + * the equivalent pointer to the same safe data.  Assumes
> + * copy_from_kernel_nofault() has already happened and our safe buffer is
> + * populated.
> + */
> +#define __btf_show_obj_safe(show, data, size)				       \
> +	(btf_show_obj_is_safe(show, data, size) ?			       \
> +	 show->obj.safe + (data - show->obj.data) : NULL)

Similarly I don't understand the benefit of macros.
They all could have been normal functions.

> +static inline void *btf_show_obj_safe(struct btf_show *show,
> +				      const struct btf_type *t,
> +				      void *data)

drop 'inline' pls.

> +{
> +	int size_left, size;
> +	void *safe = NULL;
> +
> +	if (show->flags & BTF_SHOW_UNSAFE)
> +		return data;
> +
> +	(void) btf_resolve_size(show->btf, t, &size);

Is this ok to ignore the error?