Jesper Dangaard Brouer <brouer@xxxxxxxxxx> writes:
> On Wed, 08 Apr 2020 13:53:01 +0200 Jesper Dangaard Brouer <brouer@xxxxxxxxxx> wrote:
>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 7628b947dbc3..4d58a147eed0 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3422,12 +3422,26 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
>>
>> BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
>> {
>> + void *data_hard_end = xdp_data_hard_end(xdp);
>> void *data_end = xdp->data_end + offset;
>>
> [...]
>> + /* DANGER: ALL drivers MUST be converted to init xdp->frame_sz
>> + * - Adding some chicken checks below
>> + * - Will (likely) not be for upstream
>> + */
>> + if (unlikely(xdp->frame_sz < (xdp->data_end - xdp->data_hard_start))) {
>> + WARN(1, "Too small xdp->frame_sz = %d\n", xdp->frame_sz);
>> + return -EINVAL;
>> + }
>> + if (unlikely(xdp->frame_sz > PAGE_SIZE)) {
>> + WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz);
>> + return -EINVAL;
>> + }
>
> Any opinions on above checks?
> Should they be removed or kept?
>
> The idea is to catch drivers that forgot to update xdp_buff->frame_sz,
> by doing some sanity checks on this uninit value. If I correctly
> updated all XDP drivers in this patchset, then these checks should be
> unnecessary, but will this be valuable for driver developers converting
> new drivers to XDP to have these WARN checks?
Hmm, I wonder if there's a way we could have these kinds of checks
available, but disabled by default? A new macro (e.g.,
XDP_CHECK(condition)) that is only enabled when some debug option is
enabled in the kernel build, perhaps? Or just straight ifdef'ing them
out, but maybe a macro would be generally useful?
-Toke
