Message-ID: <15676ff5-5c5c-fd06-308f-10611c01f6a9@eho.link>
Date:   Thu, 9 Dec 2021 18:55:20 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v1 bpf 1/1] libbpf: don't force user-supplied ifname
 string to be of fixed size
Content-Language: en-US
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20211209120327.551952-1-emmanuel.deloget@eho.link>
 <CAEf4BzYJ+GPpjcMMYQM_BfQ1-aq6dz_JbF-m5meiCZ=oPbrM=w@mail.gmail.com>
From:   Emmanuel Deloget <emmanuel.deloget@eho.link>
In-Reply-To: <CAEf4BzYJ+GPpjcMMYQM_BfQ1-aq6dz_JbF-m5meiCZ=oPbrM=w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RmYxQTUybnR0YnJYaDhFMEFIVlNOeEtsbzVGUS9udGFhNEZtREUzMXRxeEM3?=
 =?utf-8?B?ZnF1ZGlqRzU2bVRlRGtodTNCaUU4K3drQTZKSFA2c2FYa1NtcVg1Mk1kdzln?=
 =?utf-8?B?M25EMkNQSHpMWTdhTUR4TFNoZ01pRWR4V0t2MFRoclVzd3pSZ2NzS1g2cmgr?=
 =?utf-8?B?QlE5MDd6Z0RTUFNRdnRtUThPc2lReVpZczBpdURBV0xQcEMzVHJjR1JaV3FP?=
 =?utf-8?B?M2x6eU42eTNueGF4aUxMWlBKMFZjVmZaS2JoUWhYMXFJZXE1ZE5VYnJmSzJh?=
 =?utf-8?B?V3hKREtONGJ4US8wOURMN1d2QnJ0MEJmdXhpVkZ4L1RSTjc5N2UwLy9YRW9x?=
 =?utf-8?B?ME9pd0RKdTFpaTlUSFRMREwwcDZQdVdaWVdRczIzekxVVnFibU9rZlA0ZklB?=
 =?utf-8?B?WVVwNUJoT2tPK0xhTWM2KzhObGJhaWUzR2xPRVJHeElsNDVPYXlxZ1ZEZHF5?=
 =?utf-8?B?akQ0MWEzZFQ3a21QbXhpWlFFWUNmVWkvOGU5UEY4ZWllQ00rMGxkWDl1cUpH?=
 =?utf-8?B?NDU3QWdzMTloYXJsZ0dVbXgwRG0zYVNUdmt0bXpwcUVWS3d5NkpaUlJjcVZU?=
 =?utf-8?B?QldHbU9nOG52SUxqTlkxZy8yS00wYVAvNHNGYW5JbFdIa0wvcUc1QWorcTRW?=
 =?utf-8?B?MWZXNnRhYjZJalhqRHNDci9uOFJZelBzRnIxK2lIWkUyZ2FIUDZldEE1SCsy?=
 =?utf-8?B?QzNxaytzMTZXOEJ6M1RPcjNreXMzQVg3UzV6WGhnQlQ0Q0NQZzM1YlBMSkZo?=
 =?utf-8?B?US9kY2JsbHhEdFZKaERGVGliWkJKcVRWWkxaczJJdXUzQVhkbnhIZFdxbHR5?=
 =?utf-8?B?U1R1V0RrY002OEtIT2NjTEdIQndPaXR2Z2dJcjNMRkxZaFVhRmxXMVU1WW9L?=
 =?utf-8?B?REY2N0VzWmJkSzR1NzdOSzN5Ni9LYy8zSGtYamdMdDYwbDNCWnBKS1puc05z?=
 =?utf-8?B?d2tnd1YvQzdwZUtlbXlZM1pKQitvb1JRcGg2LzlqaE5FMHdMNWVCMXYzVWl0?=
 =?utf-8?B?a3k0aFpXUmNaN2JUL01KRXZtaWpNRS9GM0NTd0NQRTJzcjJuNk1Xc0k2SDd1?=
 =?utf-8?B?Sm9oRDA0bXlFUjdQWG9JdnNEUUlseWpFbkNWc2xuaFZKNFJRTWpWNFg0SUVI?=
 =?utf-8?B?Z01NTXZYUHBPOHgzMC9ZaE04bnpGOHphaUg3RUVGUGVTSlhiNksvL2ZUWjNH?=
 =?utf-8?B?NTlncWppWlVtZWpnUXdYeHJjUHRsQzhlL2pFdkFUSHRJYnppc09vcXNrOER3?=
 =?utf-8?B?bXZFUjF4Qi9tUVRyL1dVcXNsNHM0VlRjRjJvdnJ4d3ZreHhHczM0OGlHMElp?=
 =?utf-8?B?TmltZC91bVQ0RXUyeXNRY00xbHNITTkvZytFbnNrVnZOZHdRS2VUdXpvMlBi?=
 =?utf-8?B?bkVRY0d5clBYSTVnVkFBWHloUytKTzMrRDBGbENMTU9WVXZRNWRCZ0dhd2pV?=
 =?utf-8?B?d0xBQ0FXMHBNWjAzQWM5WUFabllDVXNFVlNRYWtTUFlSbE95RkE4QlkzZlBE?=
 =?utf-8?B?M0ZaUkJ2bG9iWGFqWTY3bHFIaThLNE4vQkpPcVVPQ3RoNkZKNG5wY3V2WEUy?=
 =?utf-8?B?eDdMNmMyYzBEdnVPN0xtc0UxeENSbUZieitzam53K0E5YnhMSGNCZzVoUjZC?=
 =?utf-8?B?eklyTmNGUzREakxwYnZXcHpnRENhYysray9KS0dZaE5jY1FFRUEvMVVTaWdy?=
 =?utf-8?B?VndaZUc2NWxZKzJVdkhUcERMaVNXU1Y4QTFzSjRuRDNZWVI0bkJwZ04yN3Vo?=
 =?utf-8?B?eUVlL0E2OHNVN01QSE5tcGlDRFNZdjFQWlVYTnZnVWIyMmJqS2REbFM5RTdt?=
 =?utf-8?B?cGVscVZOV2Zya2IycDVlOGhRVHJtdmwrRHBtUHA4TFdZNm9CUy84YUwyQSsy?=
 =?utf-8?B?Q1hDYk9abDUxdjd6MUNCdUI5LzUyNUpmMVJEdW9jbjRhVlo5L0tneURaOXBx?=
 =?utf-8?B?UUlLaDVrV09TUWNNclNiSmQrK0NQdEhHK3BSMXNaYXduYzRWb2s5aEE2blo1?=
 =?utf-8?B?VUpPa0NXVnRaSFAzaVdPUTZOTGZZNWRSTzZ0Q296anpwb0JmNm1xK2UzWFpP?=
 =?utf-8?B?U1JxZ09sU2xvanovRkYxUnFyWWNQVFhyTXdSK2NnN1ZLRXZWdXd1RDY2QWNm?=
 =?utf-8?B?akh1VHdKTFlJZTgvaG5GNVdEaHQ0VGd6anRpUUhGbjlhYnVyMFE3Q2Z5MDQx?=
 =?utf-8?B?N2FWMDVjMnlQdzJrcUVQc2JlSVFaUDJVNVQ3WWEyL0Q3RU45Uk0yQ0RvVjlH?=
 =?utf-8?B?eWdTNGNCWmtIdFc2N3lIdGdYelJMdWtxT1VaUjU4S2ZVdzFNK1BTRXJ0N0Vo?=
 =?utf-8?B?NGVWSm1HUGJOMW1kUDBhaHZxMWJSQS9HaW12QjFzbm0vT0UvTHBsZz09?=
X-OriginatorOrg: eho.link
X-MS-Exchange-CrossTenant-Network-Message-Id: c82fd07c-d1f1-44f0-9f71-08d9bb3d1225
X-MS-Exchange-CrossTenant-AuthSource: DB9PR06MB8058.eurprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 17:55:22.4696
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 924d502f-ff7e-4272-8fa5-f920518a3f4c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gPRkUR3SnDakpoA/I1o79uXr0FS9aYQ/1zdYpGq9f4Jr6SCmlRe6KMndkMtEWTbXODG0kqUZ9HoajKzzWXD2UB6L9aG2AdG0O5S67eRFg2M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR06MB6249
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

On 09/12/2021 18:17, Andrii Nakryiko wrote:
> On Thu, Dec 9, 2021 at 4:03 AM Emmanuel Deloget
> <emmanuel.deloget@eho.link> wrote:
>>
>> When calling either xsk_socket__create_shared() or xsk_socket__create()
>> the user supplies a const char *ifname which is implicitely supposed to
>> be a pointer to the start of a char[IFNAMSIZ] array. The internal
>> function xsk_create_ctx() then blindly copy IFNAMSIZ bytes from this
>> string into the xsk context.
>>
>> This is counter-intuitive and error-prone.
>>
>> For example,
>>
>>          int r = xsk_socket__create(..., "eth0", ...)
>>
>> may result in an invalid object because of the blind copy. The "eth0"
>> string might be followed by random data from the ro data section,
>> resulting in ctx->ifname being filled with the correct interface name
>> then a bunch and invalid bytes.
>>
>> The same kind of issue arises when the ifname string is located on the
>> stack:
>>
>>          char ifname[] = "eth0";
>>          int r = xsk_socket__create(..., ifname, ...);
>>
>> Or comes from the command line
>>
>>          const char *ifname = argv[n];
>>          int r = xsk_socket__create(..., ifname, ...);
>>
>> In both case we'll fill ctx->ifname with random data from the stack.
>>
>> In practice, we saw that this issue caused various small errors which,
>> in then end, prevented us to setup a valid xsk context that would have
>> allowed us to capture packets on our interfaces. We fixed this issue in
>> our code by forcing our char ifname[] to be of size IFNAMSIZ but that felt
>> weird and unnecessary.
> 
> I might be missing something, but the eth0 example above would include
> terminating zero at the right place, so ifname will still have
> "eth0\0" which is a valid string. Yes there will be some garbage after
> that, but it shouldn't matter. It could cause ASAN to complain about
> reading beyond allocated memory, of course, but I'm curious what
> problems you actually ran into in practice.

I cannot be extremely precise on what was happening as I did not 
investigate past this (and this fixes our issue) but I suspect that 
having weird bytes in ctx->ifname polutes ifr.ifr_name as initialized in 
xsk_get_max_queues(). ioctl(SIOCETHTOOL) was then giving us an error. 
Now, I haven't looked how the kernel implements this ioctl() so I'm not 
going to say that there is a problem here as well.

And since the issue is now about 2 weeks old it's now a bit murky - and 
I don't have much time to put myself in the same setup in order to 
produce a better investigation (sorry for that).

>>
>> Fixes: 2f6324a3937f8 (libbpf: Support shared umems between queues and devices)
>> Signed-off-by: Emmanuel Deloget <emmanuel.deloget@eho.link>
>> ---
>>   tools/lib/bpf/xsk.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>> index 81f8fbc85e70..8dda80bcefcc 100644
>> --- a/tools/lib/bpf/xsk.c
>> +++ b/tools/lib/bpf/xsk.c
>> @@ -944,6 +944,7 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
>>   {
>>          struct xsk_ctx *ctx;
>>          int err;
>> +       size_t ifnamlen;
>>
>>          ctx = calloc(1, sizeof(*ctx));
>>          if (!ctx)
>> @@ -965,8 +966,10 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
>>          ctx->refcount = 1;
>>          ctx->umem = umem;
>>          ctx->queue_id = queue_id;
>> -       memcpy(ctx->ifname, ifname, IFNAMSIZ - 1);
>> -       ctx->ifname[IFNAMSIZ - 1] = '\0';
>> +
>> +       ifnamlen = strnlen(ifname, IFNAMSIZ);
>> +       memcpy(ctx->ifname, ifname, ifnamlen);
> 
> maybe use strncpy instead of strnlen + memcpy? keep the guaranteed
> zero termination (and keep '\0', why did you change it?)

Well, strncpy() calls were replaced by memcpy() a while ago (see 
3015b500ae42 (libbpf: Use memcpy instead of strncpy to please GCC) for 
example but there are a few other examples ; most of the changes were 
made to please gcc8) so I thought that it would be a bad idea :). What 
would be the consensus on this?

Regarding '\0', I'll change that.

> Also, note that xsk.c is deprecated in libbpf and has been moved into
> libxdp, so please contribute a similar fix there.

Will do.

>> +       ctx->ifname[IFNAMSIZ - 1] = 0;
>>
>>          ctx->fill = fill;
>>          ctx->comp = comp;
>> --
>> 2.32.0
>>

BTW, is there a reason why this patch failed to pass the bpf/vmtest-bpf 
test on patchwork?

Best regards,

-- Emmanuel Deloget