netdev - Re: [PATCH bpf-next] selftest/bpf: check invalid length in test_xdp_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Feb 2022 09:08:33 -0800
From:   Yonghong Song <yhs@...com>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
CC:     Lorenzo Bianconi <lorenzo@...nel.org>, <bpf@...r.kernel.org>,
        <ast@...nel.org>, <daniel@...earbox.net>, <brouer@...hat.com>,
        <toke@...hat.com>, <andrii@...nel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next] selftest/bpf: check invalid length in
 test_xdp_update_frags



On 2/4/22 11:08 AM, Lorenzo Bianconi wrote:
>>>
>>
>> [...]
>>
>>>>>
>>>>> In kernel, the nr_frags checking is against MAX_SKB_FRAGS,
>>>>> but if /proc/sys/net/core/max_skb_flags is 2 or more less
>>>>> than MAX_SKB_FRAGS, the test won't fail, right?
>>>>
>>>> yes, you are right. Should we use the same definition used in
>>>> include/linux/skbuff.h instead? Something like:
>>>>
>>>> if (65536 / page_size + 1 < 16)
>>>> 	max_skb_flags = 16;
>>>> else
>>>> 	max_skb_flags = 65536/page_size + 1;
>>>
>>> The maximum packet size limit 64KB won't change anytime soon.
>>> So the above should work. Some comments to explain why using
>>> the above formula will be good.
>>
>> ack, I will do in v2.
> 
> I can see there is a on-going discussion here [0] about increasing
> MAX_SKB_FRAGS. I guess we can put on-hold this patch and see how
> MAX_SKB_FRAGS will be changed.

Thanks for the link. The new patch is going to increase
MAX_SKB_FRAGS and it is possible that will be changed again
(maybe under some config options).

The default value for
/proc/sys/net/core/max_skb_flags is MAX_SKB_FRAGS and I suspect
anybody is bothering to change it. So your patch is okay to me.
Maybe change a little bit -ENOMEM error message. current,
   ASSERT_EQ(err, -ENOMEM, "unsupported buffer size");
to
   ASSERT_EQ(err, -ENOMEM, "unsupported buffer size, possible 
non-default /proc/sys/net/core/max_skb_flags?");

> 
> Regards,
> Lorenzo
> 
> [0] https://lore.kernel.org/all/202202031315.B425Ipe8-lkp@intel.com/t/#ma1b2c7e71fe9bc69e24642a62dadf32fda7d5f03
> 
>>
>> Regards,
>> Lorenzo
>>
>>>
>>>>
>>>> Regards,
>>>> Lorenzo
>>>>
>>>>>
>>>>>> +
>>>>>> +	num = fscanf(f, "%d", &max_skb_frags);
>>>>>> +	fclose(f);
>>>>>> +
>>>>>> +	if (!ASSERT_EQ(num, 1, "max_skb_frags read failed"))
>>>>>> +		goto out;
>>>>>> +
>>>>>> +	/* xdp_buff linear area size is always set to 4096 in the
>>>>>> +	 * bpf_prog_test_run_xdp routine.
>>>>>> +	 */
>>>>>> +	buf_size = 4096 + (max_skb_frags + 1) * sysconf(_SC_PAGE_SIZE);
>>>>>> +	buf = malloc(buf_size);
>>>>>> +	if (!ASSERT_OK_PTR(buf, "alloc buf"))
>>>>>> +		goto out;
>>>>>> +
>>>>>> +	memset(buf, 0, buf_size);
>>>>>> +	offset = (__u32 *)buf;
>>>>>> +	*offset = 16;
>>>>>> +	buf[*offset] = 0xaa;
>>>>>> +	buf[*offset + 15] = 0xaa;
>>>>>> +
>>>>>> +	topts.data_in = buf;
>>>>>> +	topts.data_out = buf;
>>>>>> +	topts.data_size_in = buf_size;
>>>>>> +	topts.data_size_out = buf_size;
>>>>>> +
>>>>>> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
>>>>>> +	ASSERT_EQ(err, -ENOMEM, "unsupported buffer size");
>>>>>> +	free(buf);
>>>>>>     out:
>>>>>>     	bpf_object__close(obj);
>>>>>>     }
>>>>>
>>>
> 
>