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=-17.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 1FF12C433B4 for ; Thu, 22 Apr 2021 07:13:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DF0A46145A for ; Thu, 22 Apr 2021 07:13:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234927AbhDVHNj (ORCPT ); Thu, 22 Apr 2021 03:13:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52558 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229655AbhDVHNh (ORCPT ); Thu, 22 Apr 2021 03:13:37 -0400 Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 38A47C06138B for ; Thu, 22 Apr 2021 00:13:03 -0700 (PDT) Received: by mail-ej1-x633.google.com with SMTP id mh2so45638184ejb.8 for ; Thu, 22 Apr 2021 00:13:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Vdeyx2EiZ4VVtfgSkJvRIr5ICOqaF5aOZ8liQEySarM=; b=YPV3CqMEfMbRawW5v7Mh0/kDQEEeiRidIc6BFfEJ7Cy2RPnOBKpjJ3meCyg3B3dj7Y T23HcXybLv+3EUDMj25ODMxPRP8kuUreNtyZ4Ci8dIhpAMxALrSBE8jJrGWzTXmhxYZb Ua7sq7Z+p4ds57CAyxWZL0TtBlqKP20dnp5kg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Vdeyx2EiZ4VVtfgSkJvRIr5ICOqaF5aOZ8liQEySarM=; b=GU5yqtiVVMLg+bZ4e/VPz5ap/KX+vZZpHdXc6xbKRgrszoLfG59buDt5hWTVvn5W2V 6bWsa4hvtn60/ePs3kjln/ay0LYqnjee3Hvuqlt1F5BqufzcHyqWB1NDKDA0OJTv6N+X 32p7uW1ui5oo3sH6S326f/eUk7qaWJi4yTqr6ZbIzx/dSOk5IhRrUs3gRfeTBJzSHuIL TTngr/nKc7TtP8Wriq4ic5dofMDDaCT7MVCcmbHWX8Bbcy3+XmUz+H2A8SW2etOVUow5 RjJdSVOQsRYJ9lvt5ssJwE01tMh1u2zDDnXREtcP0v4oLesevsXSOIhgSIoug01ghHmk uW3w== X-Gm-Message-State: AOAM531Or9VwYKKEs4mX/BAp+MeKaKWlJNTWmq6PiVgerzmxdnV3v9sl 0ejJRILgJd0K9HPRp8t94vRnfA== X-Google-Smtp-Source: ABdhPJzmo155Rt45+yFK2rxmK+jof0hG9KwOE3VJP8U8oPglLPeWzFmQpm5PJGsvjv1Y0WrrLAtA/g== X-Received: by 2002:a17:906:9990:: with SMTP id af16mr1776543ejc.195.1619075581971; Thu, 22 Apr 2021 00:13:01 -0700 (PDT) Received: from [192.168.1.149] ([80.208.71.248]) by smtp.gmail.com with ESMTPSA id ca1sm1248712edb.76.2021.04.22.00.13.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 22 Apr 2021 00:13:01 -0700 (PDT) Subject: Re: [PATCH] bpf: remove pointless code from bpf_do_trace_printk() To: Andrii Nakryiko Cc: Daniel Borkmann , Alan Maguire , Steven Rostedt , bpf , open list , Florent Revest , Alexei Starovoitov References: <20210421190736.1538217-1-linux@rasmusvillemoes.dk> From: Rasmus Villemoes Message-ID: <236995f6-30ee-8047-624c-08d0a1552dc1@rasmusvillemoes.dk> Date: Thu, 22 Apr 2021 09:13:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/04/2021 05.32, Andrii Nakryiko wrote: > On Wed, Apr 21, 2021 at 6:19 PM Rasmus Villemoes > wrote: >> >> The comment is wrong. snprintf(buf, 16, "") and snprintf(buf, 16, >> "%s", "") etc. will certainly put '\0' in buf[0]. The only case where >> snprintf() does not guarantee a nul-terminated string is when it is >> given a buffer size of 0 (which of course prevents it from writing >> anything at all to the buffer). >> >> Remove it before it gets cargo-culted elsewhere. >> >> Signed-off-by: Rasmus Villemoes >> --- >> kernel/trace/bpf_trace.c | 3 --- >> 1 file changed, 3 deletions(-) >> > > The change looks good to me, but please rebase it on top of the > bpf-next tree. This is not a bug, so it doesn't have to go into the > bpf tree. As it is right now, it doesn't apply cleanly onto bpf-next. Thanks for the pointer. Looking in next-20210420, it seems to me that commit d9c9e4db186ab4d81f84e6f22b225d333b9424e3 Author: Florent Revest Date: Mon Apr 19 17:52:38 2021 +0200 bpf: Factorize bpf_trace_printk and bpf_seq_printf is buggy. In particular, these two snippets: +#define BPF_CAST_FMT_ARG(arg_nb, args, mod) \ + (mod[arg_nb] == BPF_PRINTF_LONG_LONG || \ + (mod[arg_nb] == BPF_PRINTF_LONG && __BITS_PER_LONG == 64) \ + ? (u64)args[arg_nb] \ + : (u32)args[arg_nb]) + ret = snprintf(buf, sizeof(buf), fmt, BPF_CAST_FMT_ARG(0, args, mod), + BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod)); Regardless of the casts done in that macro, the type of the resulting expression is that resulting from C promotion rules. And (foo ? (u64)bla : (u32)blib) has type u64, which is thus the type the compiler uses when building the vararg list being passed into snprintf(). C simply doesn't allow you to change types at run-time in this way. It probably works fine on x86-64, which passes the first six or so argument in registers, va_start() puts those registers into the va_list opaque structure, and when it comes time to do a va_arg(int), just the lower 32 bits are used. It is broken on i386 and other architectures where arguments are passed on the stack (and for x86-64 as well had there been a few more arguments) and va_arg(ap, int) is essentially ({ int res = *(int *)ap; ap += 4; res; }) [or maybe it's -= 4 because stack direction etc., that's not really relevant here]. Rasmus