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=-5.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 536C4C433ED for ; Wed, 14 Apr 2021 09:46:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2B17761220 for ; Wed, 14 Apr 2021 09:46:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231892AbhDNJrN (ORCPT ); Wed, 14 Apr 2021 05:47:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233066AbhDNJrK (ORCPT ); Wed, 14 Apr 2021 05:47:10 -0400 Received: from mail-io1-xd35.google.com (mail-io1-xd35.google.com [IPv6:2607:f8b0:4864:20::d35]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C2137C061756 for ; Wed, 14 Apr 2021 02:46:49 -0700 (PDT) Received: by mail-io1-xd35.google.com with SMTP id j26so20029216iog.13 for ; Wed, 14 Apr 2021 02:46:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=QSY+YF0ygYiG1EiJZskRZOjnmmttz2RNZ0RIR9uNbSI=; b=k7ieuvix8d6RycI66YKCN8yW4dVTY36rKSS/XWuHxyv0q5bdN0h1ICUXSNyrvs4F3o +Xyv5f/8HrExwKjEwsVFfcgbKVSYiqSM6jf3ftSqntLpKcSmyrKOmbNcgHlfkC7VC2B5 qTKlt2g5WLJo98c3Bm+C1av3VrY3wE0hfPcNQ= 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=QSY+YF0ygYiG1EiJZskRZOjnmmttz2RNZ0RIR9uNbSI=; b=rKcJKO0bHeubK3EzxtBcy7RRGI1Xc3cTDxZ7Nyqzt1sELnvq8jWBXRU2qw/GjkRtKK PYF0kLaSwlCn/1RnOm8Qx6uCnmM73hOGOY3+Dc5bg/5UAYAxySQns4HZDIKo2XWVoH6V cj7v0Ixbu6edlAEy1Kx5yNmxqYISsl8xdI4rfbS00sVuxma+ECxZC8WV5Cz4mDtCVkxE DkCj3wdcZT3cNJurvv9mxhxYCR1+9q/PAAS6JKUJLwxAEMilzzU45Ru/W+4aJ/VkczMU BzjRIo1FSIeA0/3dyZyd82hSK0IVa1FgSBR2oKqzQmJO9b6Ud//bYOqALEemOBzeJmm2 abTg== X-Gm-Message-State: AOAM533uUNwSunSIdagGH9FR2QRnftRmcbNMm/WJIgVqgbjOkBRLaZLh WwSv+dPCn2YeWfV4Lc1jW86Tg9kkAx0HXXbcnTCBLWBQ4EiS6w== X-Google-Smtp-Source: ABdhPJzPixnojwlN8XP8k88cP6u03RzsNaQL6/YxmSt+0RNWzwVpls932y0asMJtoxu14QXvEPsckB3We2i+WUq90JU= X-Received: by 2002:a02:cf16:: with SMTP id q22mr25911665jar.32.1618393609117; Wed, 14 Apr 2021 02:46:49 -0700 (PDT) MIME-Version: 1.0 References: <20210412153754.235500-1-revest@chromium.org> <20210412153754.235500-4-revest@chromium.org> In-Reply-To: From: Florent Revest Date: Wed, 14 Apr 2021 11:46:38 +0200 Message-ID: Subject: Re: [PATCH bpf-next v3 3/6] bpf: Add a bpf_snprintf helper To: Andrii Nakryiko Cc: bpf , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Yonghong Song , KP Singh , Brendan Jackman , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 14, 2021 at 1:16 AM Andrii Nakryiko wrote: > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest wrote: > > +static int check_bpf_snprintf_call(struct bpf_verifier_env *env, > > + struct bpf_reg_state *regs) > > +{ > > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; > > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; > > + struct bpf_map *fmt_map = fmt_reg->map_ptr; > > + int err, fmt_map_off, num_args; > > + u64 fmt_addr; > > + char *fmt; > > + > > + /* data must be an array of u64 */ > > + if (data_len_reg->var_off.value % 8) > > + return -EINVAL; > > + num_args = data_len_reg->var_off.value / 8; > > + > > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const > > + * and map_direct_value_addr is set. > > + */ > > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value; > > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr, > > + fmt_map_off); > > + if (err) > > + return err; > > + fmt = (char *)fmt_addr + fmt_map_off; > > + > > bot complained about lack of (long) cast before fmt_addr, please address Will do. > > + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give > > + * all of them to snprintf(). > > + */ > > + err = snprintf(str, str_size, fmt, BPF_CAST_FMT_ARG(0, args, mod), > > + BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod), > > + BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod), > > + BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod), > > + BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod), > > + BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod), > > + BPF_CAST_FMT_ARG(11, args, mod)); > > + > > + put_fmt_tmp_buf(); > > reading this for at least 3rd time, this put_fmt_tmp_buf() looks a bit > out of place and kind of random. I think bpf_printf_cleanup() name > pairs with bpf_printf_prepare() better. Yes, I thought it would be clever to name that function put_fmt_tmp_buf() as a clear parallel to try_get_fmt_tmp_buf() but because it only puts the buffer if it is used and because they get called in two different contexts, it's after all maybe not such a clever name... I'll revert to bpf_printf_cleanup(). Thank you for your patience with my naming adventures! :) > > + > > + return err + 1; > > snprintf() already returns string length *including* terminating zero, > so this is wrong lib/vsprintf.c says: * The return value is the number of characters which would be * generated for the given input, excluding the trailing null, * as per ISO C99. Also if I look at the "no arg" test case in the selftest patch. "simple case" is asserted to return 12 which seems correct to me (includes the terminating zero only once). Am I missing something ? However that makes me wonder whether it would be more appropriate to return the value excluding the trailing null. On one hand it makes sense to be coherent with other BPF helpers that include the trailing zero (as discussed in patch v1), on the other hand the helper is clearly named after the standard "snprintf" function and it's likely that users will assume it works the same as the std snprintf.