From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F30A8C433ED for ; Wed, 28 Apr 2021 08:18:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B2F9D61154 for ; Wed, 28 Apr 2021 08:18:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237157AbhD1IS7 (ORCPT ); Wed, 28 Apr 2021 04:18:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35574 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230456AbhD1IS6 (ORCPT ); Wed, 28 Apr 2021 04:18:58 -0400 Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 11993C061574 for ; Wed, 28 Apr 2021 01:18:12 -0700 (PDT) Received: by mail-oi1-x235.google.com with SMTP id m13so62264596oiw.13 for ; Wed, 28 Apr 2021 01:18:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+YTrfob6P6zzC8aXEunzXLUOwk7qpHbcRbQ3sHEGAGI=; b=KcF92B5WtEXaqweynQ3beawG/4jC+fJUKiBJErUOLSrVTR7P/T3+noJZGR9sfZHfeE rCwoFVOXaUuPdSFjs9G0zBF8QlI9brCPN1jHJlCgIfJjdW13hPkhOjBepQ0F6vblyosE Fi6wuv6B7QP1SfQqh12985hzP7/bGDMx1inwfrQxdVMibui/oBYRbpNzeSJLx8l4eDPj w8AZfzOhhUryHMsaVuP9Eom4CuoZ4wTSHeft1SzuEEzau6shOwyZohMWsC0p/jR57Vm5 xQCrM0paYd31A7UOQNIverysqYeuCeiYjAv4C84AG5TfuQ81/QN1TuJPOCNmgPNXiore f5IQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+YTrfob6P6zzC8aXEunzXLUOwk7qpHbcRbQ3sHEGAGI=; b=ae7+2YYc0zqfuYQlZRHqksAeP4w1zOG33gDSh8tnc9AhS5o8oR1wXyYDNDchtnB8jG MrtQl+20XA1WPxCQAZjSWvIgGxZMxXN1q+3TBNfnOcWGKmWDAjfibt8dyJ61eg9JAXH8 p+/J+xQTmC/v3ga6VxGoEUXFNL9/2oEQ84UwHSSHx/YX3AV+XK2p1uB8RtiP3khOAuQe RI2ksagMKBWzrw1KJrKiHskqK2oh2a2X6rEwNZeuU+RK0Q1u8jAv/rkfckO/YCVvIntJ vDk4fF4ouVXuoQjjIfIq7hJ/GefI09/GfxW2Z9ItX1MsiH+scq4X5pG64v/Z4nVEPnlR Rimw== X-Gm-Message-State: AOAM533hcmtJjADzuwDy+qjT+HecG4zLeTioeXA3qG0vOxLbXNeWaIKz 1rWkgO1sowSsO9wE27YU3v0Mc1FUqmDoIvILV5T8tw== X-Google-Smtp-Source: ABdhPJxgVKIvTbSoIhoIhmCiQYRpY9snoNmhchOKqCzYss7z2RuFwwa/Rhr/Dp9oFpDYlCTffC2s96FRGc3rZsRselU= X-Received: by 2002:a54:4704:: with SMTP id k4mr1876288oik.127.1619597891309; Wed, 28 Apr 2021 01:18:11 -0700 (PDT) MIME-Version: 1.0 References: <20210427170859.579924-1-jackmanb@google.com> In-Reply-To: From: Brendan Jackman Date: Wed, 28 Apr 2021 10:18:00 +0200 Message-ID: Subject: Re: [PATCH bpf-next] libbpf: Fix signed overflow in ringbuf_process_ring To: Andrii Nakryiko Cc: KP Singh , bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 28 Apr 2021 at 01:19, Andrii Nakryiko wrote: > > On Tue, Apr 27, 2021 at 4:05 PM KP Singh wrote: > > > > On Tue, Apr 27, 2021 at 11:34 PM Andrii Nakryiko > > wrote: > > > > > > On Tue, Apr 27, 2021 at 10:09 AM Brendan Jackman wrote: > > > > > > > > One of our benchmarks running in (Google-internal) CI pushes data > > > > through the ringbuf faster than userspace is able to consume > > > > it. In this case it seems we're actually able to get >INT_MAX entries > > > > in a single ringbuf_buffer__consume call. ASAN detected that cnt > > > > overflows in this case. > > > > > > > > Fix by just setting a limit on the number of entries that can be > > > > consumed. > > > > > > > > Fixes: bf99c936f947 (libbpf: Add BPF ring buffer support) > > > > Signed-off-by: Brendan Jackman > > > > --- > > > > tools/lib/bpf/ringbuf.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c > > > > index e7a8d847161f..445a21df0934 100644 > > > > --- a/tools/lib/bpf/ringbuf.c > > > > +++ b/tools/lib/bpf/ringbuf.c > > > > @@ -213,8 +213,8 @@ static int ringbuf_process_ring(struct ring* r) > > > > do { > > > > got_new_data = false; > > > > prod_pos = smp_load_acquire(r->producer_pos); > > > > - while (cons_pos < prod_pos) { > > > > + /* Don't read more than INT_MAX, or the return vale won't make sense. */ > > > > + while (cons_pos < prod_pos && cnt < INT_MAX) { > > > > > > ring_buffer__pool() is assumed to not return until all the enqueued > > > messages are consumed. That's the requirement for the "adaptive" > > > notification scheme to work properly. So this will break that and > > > cause the next ring_buffer__pool() to never wake up. Ah yes, good point, thanks. > > > We could use __u64 internally and then cap it to INT_MAX on return > > > maybe? But honestly, this sounds like an artificial corner case, if > > > you are producing data faster than you can consume it and it goes > > > beyond INT_MAX, something is seriously broken in your application and Yes it's certainly artificial but IMO it's still highly desirable for libbpf to hold up its side of the bargain even when the application is behaving very strangely like this. [...] > I think we have two alternatives here: > 1) consume all but cap return to INT_MAX > 2) consume all but return long long as return result > > Third alternative is to have another API with maximum number of > samples to consume. But then user needs to know what they are doing > (e.g., they do FORCE on BPF side, or they do their own epoll_wait, or > they do ring_buffer__poll with timeout = 0, etc). > > I'm just not sure anyone would want to understand all the > implications. And it's easy to miss those implications. So maybe let's > do long long (or __s64) return type instead? Wouldn't changing the API to 64 bit return type break existing users on some ABIs? I think capping the return value to INT_MAX and adding a note to the function definition comment would also be fine, it doesn't feel like a very complex thing for the user to understand: "Returns number of records consumed (or INT_MAX, whichever is less)".