On Fri, Mar 13, 2020 at 05:16:55PM -0700, Andrii Nakryiko wrote:
> On Fri, Mar 13, 2020 at 4:37 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
> >
> > The bpf_struct_ops tcp-cc name should be sanitized in order to
> > avoid problematic chars (e.g. whitespaces).
> >
> > This patch reuses the bpf_obj_name_cpy() for accepting the same set
> > of characters in order to keep a consistent bpf programming experience.
> > A "size" param is added. Also, the strlen is returned on success so
> > that the caller (like the bpf_tcp_ca here) can error out on empty name.
> > The existing callers of the bpf_obj_name_cpy() only need to change the
> > testing statement to "if (err < 0)". For all these existing callers,
> > the err will be overwritten later, so no extra change is needed
> > for the new strlen return value.
> >
> > Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
> > Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> > ---
> > include/linux/bpf.h | 1 +
> > kernel/bpf/syscall.c | 24 +++++++++++++-----------
> > net/ipv4/bpf_tcp_ca.c | 7 ++-----
> > 3 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 49b1a70e12c8..212991f6f2a5 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -160,6 +160,7 @@ static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
> > }
> > void copy_map_value_locked(struct bpf_map *map, void *dst, void *src,
> > bool lock_src);
> > +int bpf_obj_name_cpy(char *dst, const char *src, unsigned int size);
> >
> > struct bpf_offload_dev;
> > struct bpf_offloaded_map;
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 0c7fb0d4836d..d2984bf362c2 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -696,14 +696,14 @@ int bpf_get_file_flag(int flags)
> > offsetof(union bpf_attr, CMD##_LAST_FIELD) - \
> > sizeof(attr->CMD##_LAST_FIELD)) != NULL
> >
> > -/* dst and src must have at least BPF_OBJ_NAME_LEN number of bytes.
> > - * Return 0 on success and < 0 on error.
> > +/* dst and src must have at least "size" number of bytes.
> > + * Return strlen on success and < 0 on error.
> > */
> > -static int bpf_obj_name_cpy(char *dst, const char *src)
> > +int bpf_obj_name_cpy(char *dst, const char *src, unsigned int size)
> > {
> > - const char *end = src + BPF_OBJ_NAME_LEN;
> > + const char *end = src + size;
> >
> > - memset(dst, 0, BPF_OBJ_NAME_LEN);
> > + memset(dst, 0, size);
> > /* Copy all isalnum(), '_' and '.' chars. */
> > while (src < end && *src) {
> > if (!isalnum(*src) &&
> > @@ -712,11 +712,11 @@ static int bpf_obj_name_cpy(char *dst, const char *src)
> > *dst++ = *src++;
> > }
> >
> > - /* No '\0' found in BPF_OBJ_NAME_LEN number of bytes */
> > + /* No '\0' found in "size" number of bytes */
> > if (src == end)
> > return -EINVAL;
> >
> > - return 0;
> > + return src - (end - size);
>
> it's a rather convoluted way of writing (src - orig_src), maybe just
> remember original src?
Sure. I can send v2. Thanks for the review!
>
> Either way not a big deal:
>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
>
> > }
> >
>
> [...]
