Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname

From: Yonghong Song <yhs@fb.com>
To: Wenbo Zhang <ethercflow@gmail.com>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Cc: "ast@kernel.org" <ast@kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"bgregg@netflix.com" <bgregg@netflix.com>,
	"andrii.nakryiko@gmail.com" <andrii.nakryiko@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
Date: Wed, 18 Dec 2019 03:27:09 +0000	[thread overview]
Message-ID: <4e09844f-8904-5d84-c25d-6fc0cf60b8b7@fb.com> (raw)
In-Reply-To: <7464919bd9c15f2496ca29dceb6a4048b3199774.1576629200.git.ethercflow@gmail.com>


On 12/17/19 4:56 PM, Wenbo Zhang wrote:
> When people want to identify which file system files are being opened,
> read, and written to, they can use this helper with file descriptor as
> input to achieve this goal. Other pseudo filesystems are also supported.
> 
> This requirement is mainly discussed here:
> 
>    https://github.com/iovisor/bcc/issues/237
> 
> v13->v14: addressed Yonghong and Daniel's feedback
> - fix this helper's description to be consistent with comments in d_path
> - fix error handling logic fill zeroes not '0's
> 
> v12->v13: addressed Brendan and Yonghong's feedback
> - rename to get_fd_path
> - refactor code & comment to be clearer and more compliant
> 
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
> 
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
> 
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
> 
> v8->v9:
> - format helper description
> 
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
> 
> v6->v7:
> - fix missing signed-off-by line
> 
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
> 
> v4->v5: addressed Andrii and Daniel's feedback
> - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> helper's names
> - when fdget_raw fails, set ret to -EBADF instead of -EINVAL
> - remove fdput from fdget_raw's error path
> - use IS_ERR instead of IS_ERR_OR_NULL as d_path ether returns a pointer
> into the buffer or an error code if the path was too long
> - modify the normal path's return value to return copied string length
> including NUL
> - update this helper description's Return bits.
> 
> v3->v4: addressed Daniel's feedback
> - fix missing fdput()
> - move fd2path from kernel/bpf/trace.c to kernel/trace/bpf_trace.c
> - move fd2path's test code to another patch
> - add comment to explain why use fdget_raw instead of fdget
> 
> v2->v3: addressed Yonghong's feedback
> - remove unnecessary LOCKDOWN_BPF_READ
> - refactor error handling section for enhanced readability
> - provide a test case in tools/testing/selftests/bpf
> 
> v1->v2: addressed Daniel's feedback
> - fix backward compatibility
> - add this helper description
> - fix signed-off name
> 
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

Acked-by: Yonghong Song <yhs@fb.com>