Hi,

On 11/7/2021 3:26 AM, Alexei Starovoitov wrote:

On Sat, Nov 06, 2021 at 09:28:21PM +0800, Hou Tao wrote:

quoted

The helper compares two strings: one string is a null-terminated
read-only string, and another one has const max storage size. And
it can be used to compare file name in tracing or LSM program.

We don't check whether or not s2 in bpf_strncmp() is null-terminated,
because its content may be changed by malicous program, and we only
ensure the memory accessed is bounded by s2_sz.

I think "malicous" adjective is unnecessary and misleading.
It's also misspelled.
Just mention that 2nd argument doesn't have to be null terminated.

Will do.

quoted

+ * long bpf_strncmp(const char *s1, const char *s2, u32 s2_sz)

...

quoted

+BPF_CALL_3(bpf_strncmp, const char *, s1, const char *, s2, size_t, s2_sz)

probably should match u32 instead of size_t.

Yes, will use u32 instead. I forgot to synchronize between these two declarations.

quoted

@@ -1210,6 +1210,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_get_branch_snapshot_proto;
 	case BPF_FUNC_trace_vprintk:
 		return bpf_get_trace_vprintk_proto();
+	case BPF_FUNC_strncmp:
+		return &bpf_strncmp_proto;

why tracing only?
Should probably be in bpf_base_func_proto.

Because in our use case, bpf_strncmp() is only used by tracing program, but moving
it to bpf_base_func_proto() incurs no harm, so will do it.

I was thinking whether the proto could be:
long bpf_strncmp(const char *s1, u32 s1_sz, const char *s2)
but I think your version is better though having const string as 1st arg
is a bit odd in normal C.

Would it make sense to add bpf_memchr as well while we are at it?
And
static inline bpf_strnlen(const char *s, u32 sz)
{
  return bpf_memchr(s, sz, 0);
}
to bpf_helpers.h ?
.

It is OK to add it, although I don't have a use case for it.