On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
> If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> called unconditionally on skb_verdict, even though it may be NULL. Fix
> and tidy up error path.
>
> Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> Signed-off-by: Alex Dewar <[email protected]>
> ---
Acked-by: Jakub Sitnicki <[email protected]>
Jakub Sitnicki wrote:
> On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
> > If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> > called unconditionally on skb_verdict, even though it may be NULL. Fix
> > and tidy up error path.
> >
> > Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> > Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> > Signed-off-by: Alex Dewar <[email protected]>
> > ---
>
> Acked-by: Jakub Sitnicki <[email protected]>
Thanks.
Jakub, any opinions on if we should just throw an error if users try to
add a sock to a map with a parser but no verdict? At the moment we fall
through and add the socket, but it wont do any receive parsing/verdict.
At the moment I think its fine with above fix. The useful cases for RX
are parser+verdict, verdict, and empty. Where empty is just used for
redirects or other socket account tricks. Just something to keep in mind.
Acked-by: John Fastabend <[email protected]>
On Thu, Oct 15, 2020 at 06:43 AM CEST, John Fastabend wrote:
[...]
> Jakub, any opinions on if we should just throw an error if users try to
> add a sock to a map with a parser but no verdict? At the moment we fall
> through and add the socket, but it wont do any receive parsing/verdict.
> At the moment I think its fine with above fix. The useful cases for RX
> are parser+verdict, verdict, and empty. Where empty is just used for
> redirects or other socket account tricks. Just something to keep in mind.
IMO we should not fail because map updates can interleave with sk_skb
prog attachments, like so:
update_map(map_fd, sock_fd);
attach_prog(parser_fd, map_fd, BPF_SK_SKB_STREAM_PARSER);
update_map(map_fd, sock_fd); // OK
attach_prog(verdict_fd, map_fd, BPF_SK_SKB_STREAM_VERDICT);
update_map(map_fd, sock_fd);
In practice, I would expect one process/thread to attach the programs,
while another is allowed to update the map at the same time.