[linux-morello] Re: [PATCH v4 09/13] bpf: compat64: support CHECK_ATTR macro

On 06/11/2023 11:26, Kevin Brodsky wrote:

On 30/10/2023 13:48, Zachary Leaf wrote:

...

+static int check_attr(int cmd, void *vattr) +{

• switch (cmd) {
• case BPF_MAP_CREATE:

• return CHECK_ATTR(BPF_MAP_CREATE);
  

• case BPF_MAP_LOOKUP_ELEM:

• return CHECK_ATTR(BPF_MAP_LOOKUP_ELEM);
  

• case BPF_MAP_GET_NEXT_KEY:

• return CHECK_ATTR(BPF_MAP_GET_NEXT_KEY);
  

• case BPF_MAP_FREEZE:

• return CHECK_ATTR(BPF_MAP_FREEZE);
  

• case BPF_PROG_LOAD:

• return CHECK_ATTR(BPF_PROG_LOAD);
  

• case BPF_OBJ_PIN:

• return CHECK_ATTR(BPF_OBJ);
  

• case BPF_OBJ_GET:

• return CHECK_ATTR(BPF_OBJ);
  

• case BPF_PROG_ATTACH:

• return CHECK_ATTR(BPF_PROG_ATTACH);
  

• case BPF_PROG_DETACH:

• return CHECK_ATTR(BPF_PROG_DETACH);
  

• case BPF_PROG_QUERY:

• return CHECK_ATTR(BPF_PROG_QUERY);
  

• case BPF_PROG_TEST_RUN:

• return CHECK_ATTR(BPF_PROG_TEST_RUN);
  

• case BPF_PROG_GET_NEXT_ID:

• return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID);
  

• case BPF_MAP_GET_NEXT_ID:

• return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID);
  

• case BPF_BTF_GET_NEXT_ID:

• return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID);
  

• case BPF_PROG_GET_FD_BY_ID:

• return CHECK_ATTR(BPF_PROG_GET_FD_BY_ID);
  

• case BPF_MAP_GET_FD_BY_ID:

• return CHECK_ATTR(BPF_MAP_GET_FD_BY_ID);
  

• case BPF_OBJ_GET_INFO_BY_FD:

• return CHECK_ATTR(BPF_OBJ_GET_INFO_BY_FD);
  

• case BPF_RAW_TRACEPOINT_OPEN:

• return CHECK_ATTR(BPF_RAW_TRACEPOINT_OPEN);
  

• case BPF_BTF_LOAD:

• return CHECK_ATTR(BPF_BTF_LOAD);
  

• case BPF_BTF_GET_FD_BY_ID:

• return CHECK_ATTR(BPF_BTF_GET_FD_BY_ID);
  

• case BPF_TASK_FD_QUERY:

• return CHECK_ATTR(BPF_TASK_FD_QUERY);
  

• case BPF_MAP_LOOKUP_AND_DELETE_ELEM:

• return CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM);
  

• case BPF_LINK_CREATE:

• return CHECK_ATTR(BPF_LINK_CREATE);
  

• case BPF_LINK_UPDATE:

• return CHECK_ATTR(BPF_LINK_UPDATE);
  

• case BPF_LINK_GET_FD_BY_ID:

• return CHECK_ATTR(BPF_LINK_GET_FD_BY_ID);
  

• case BPF_LINK_GET_NEXT_ID:

• return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID);
  

• case BPF_ENABLE_STATS:

• return CHECK_ATTR(BPF_ENABLE_STATS);
  

• case BPF_ITER_CREATE:

• return CHECK_ATTR(BPF_ITER_CREATE);
  

• case BPF_LINK_DETACH:

• return CHECK_ATTR(BPF_LINK_DETACH);
  

• case BPF_PROG_BIND_MAP:

• return CHECK_ATTR(BPF_PROG_BIND_MAP);
  

• default:

• return 0;
  

This gets me thinking: assuming we are supposed to handle all the bpf_cmd enum values (I think we do), it would actually be better not to have a default. This way, we would get a warning if a new value is added (I think -Wswitch is enabled but worth checking this actually works).

We are meant to handle all values. To get the warning we need to change the signature to accept enum bpf_cmd instead of int cmd.

This shows us we're already missing 6 values (!)

kernel/bpf/syscall.c:5201:10: warning: 6 enumeration values not handled in switch: 'BPF_MAP_UPDATE_ELEM', 'BPF_MAP_DELETE_ELEM', 'BPF_MAP_LOOKUP_BATCH'... [-Wswitch]

Somehow missed these before. The check is good. Fixed in v5.

Thanks, Zach

Kevin