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=-0.9 required=3.0 tests=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 DF394C3437B for ; Fri, 13 Dec 2019 20:41:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1E32524784 for ; Fri, 13 Dec 2019 20:41:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=netflix.com header.i=@netflix.com header.b="itDZNQlH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728972AbfLMTvu (ORCPT ); Fri, 13 Dec 2019 14:51:50 -0500 Received: from mail-qt1-f195.google.com ([209.85.160.195]:36838 "EHLO mail-qt1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728932AbfLMTvu (ORCPT ); Fri, 13 Dec 2019 14:51:50 -0500 Received: by mail-qt1-f195.google.com with SMTP id q20so42917qtp.3 for ; Fri, 13 Dec 2019 11:51:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netflix.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RcocnBgthkpbK/m8uz5lL4Etu89gMRTjWvFTOte2r2c=; b=itDZNQlHPhHUrHqVid9ZEL0BBIhzfoYb+mAnZJJEhu2kbVcTJwANnDhQQ2vtWotOfg GuDvebWKQEIA+A5n+auB+OzFLNBHhSec5TBo4mBEwZ5Lwtyw2G4oUiVdU0dDIURlM+5J hSoDs0DqyA/KAgt/Ube70t5A/PU6LV01m7bEI= 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=RcocnBgthkpbK/m8uz5lL4Etu89gMRTjWvFTOte2r2c=; b=C0yZoNCFdQmw4vYKmd6B2pCs1qxpTaUe+aHEuOKk0rPGV/08UfhH9KthrHAeu2LVJu UAHfOLTZqh9xIlk+7reoVc1dfuhSFDJADuSgeK1KkrxSuFhzKhudBR+c0m/bbnNn/fsL MscmcnphPDtXRiNnBRous9doaTWLC1zNhQznRyuHIVxaPaN5Y2OxCHhmCI9oWKf3fs7H 7Kw4ni1rDPwtgjETtrSr13v431+x50vVifn4IsEra3NDAsXW7Dhgxc7XEI2DQu5kI4HS Bfj+BhXAQqOPzEvaqGk8/uwcy9V1RiX5kMFJeg56enhykbY5RPsSQ0Q6Qqh+orhAgBjF kaIQ== X-Gm-Message-State: APjAAAU7WF/koF5VX2wwrbEaTKrFJPEPszp8BCAtcs38aABMnf+lxebO d6irE+9Q805Yr9y2UXSnBG8XOEgYpwMOAwb7siu4MClB8iM= X-Google-Smtp-Source: APXvYqzykKfxjJE3Fx4RtcLW6QEH5ZxzYS0hScq/NEmLrnIUP+d6AApkHTcBQbyzzoCSMUgoB+AcM5H1ZItGDJ6F/io= X-Received: by 2002:aed:3ee5:: with SMTP id o34mr14102843qtf.164.1576266709314; Fri, 13 Dec 2019 11:51:49 -0800 (PST) MIME-Version: 1.0 References: <20191123031826.j2dj7mzto57ml6pr@ast-mbp.dhcp.thefacebook.com> <20191123045151.GH26530@ZenIV.linux.org.uk> <20191123051919.dsw7v6jyad4j4ilc@ast-mbp.dhcp.thefacebook.com> <20191123053514.GJ26530@ZenIV.linux.org.uk> <20191123060448.7crcqwkfmbq3gsze@ast-mbp.dhcp.thefacebook.com> In-Reply-To: <20191123060448.7crcqwkfmbq3gsze@ast-mbp.dhcp.thefacebook.com> From: Brendan Gregg Date: Fri, 13 Dec 2019 11:51:23 -0800 Message-ID: Subject: Re: [PATCH bpf-next v10 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname To: Alexei Starovoitov Cc: Al Viro , Wenbo Zhang , bpf@vger.kernel.org, ast@kernel.org.com, Daniel Borkmann , Yonghong Song , andrii.nakryiko@gmail.com, netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Fri, Nov 22, 2019 at 10:05 PM Alexei Starovoitov wrote: > > On Sat, Nov 23, 2019 at 05:35:14AM +0000, Al Viro wrote: > > On Fri, Nov 22, 2019 at 09:19:21PM -0800, Alexei Starovoitov wrote: > > > > > hard to tell. It will be run out of bpf prog that attaches to kprobe or > > > tracepoint. What is the concern about locking? > > > d_path() doesn't take any locks and doesn't depend on any locks. Above 'if' > > > checks that plain d_path() is used and not some specilized callback with > > > unknown logic. > > > > It sure as hell does. It might end up taking rename_lock and/or mount_lock > > spinlock components. It'll try not to, but if the first pass ends up with > > seqlock mismatch, it will just grab the spinlock the second time around. > > ohh. got it. I missed _or_lock() part in there. > The need_seqretry() logic is tricky. afaics there is no way for the checks > outside of prepend_path() to prevent spin_lock to happen. And adding a flag to > prepend_path() to return early if retry is needed is too ugly. So this helper > won't be safe to be run out of kprobe. But if we allow it for tracepoints only > it should be ok. I think. There are no tracepoints in inner guts of vfs and I > don't think they will ever be. So running in tracepoint->bpf_prog->d_path we > will be sure that rename_lock+mount_lock can be safely spinlocked. Am I missing > something? It seems rather restrictive to only allow tracepoints (especially without VFS tracepoints), although I'll use it to improve my syscall tracepoint tools, so I'd be happy to see this merged even with that restriction. Just a thought: if *buffer is in BPF memory, can prepend_path() check it's memory location and not try to grab the lock based on that? This would be to avoid adding a flag. > > > > > with this number; quite possibly never before that function had been called > > > > _and_ not once after it has returned. > > > > > > Right. TOCTOU is not a concern here. It's tracing. It's ok for full path to be > > > 'one time deal'. > > > > It might very well be a full path of something completely unrelated to what > > the syscall ends up operating upon. It's not that the file might've been > > moved; it might be a different file. IOW, results of that tracing might be > > misleading. > > That is correct. Tracing is fine with such limitation. Still better than probe_read. > +1 Tracing is observability tools and we document these caveats, and this won't be the first time I've published tools where the printed path may not be the one you think (e.g., the case of hard links.) Brendan -- Brendan Gregg, Senior Performance Architect, Netflix