On Sat, Nov 6, 2021 at 12:26 PM Alexei Starovoitov
[off-list ref] 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.

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.

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.

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.

Why do you think it's better? This is equivalent to `123 == x` if it
was integer comparison, so it feels like bpf_strncmp(s, sz, "blah") is
indeed more natural. No big deal, just curious what's better about it.

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 ?