bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs
@ 2020-11-12 20:09 Florent Revest
  2020-11-12 20:28 ` saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs) Al Viro
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Florent Revest @ 2020-11-12 20:09 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, kafai, yhs, andrii, kpsingh, jackmanb, linux-kernel,
	Florent Revest

From: Florent Revest <revest@google.com>

eBPF programs can already check whether a file is a socket using
file->f_op == &socket_file_ops but they can not convert file->private_data
into a struct socket with BTF information. For that, we need a new
helper that is essentially just a wrapper for sock_from_file.

sock_from_file can set an err value but this is only set to -ENOTSOCK
when the return value is NULL so it's useless superfluous information.

Signed-off-by: Florent Revest <revest@google.com>
---
 include/uapi/linux/bpf.h       |  7 +++++++
 kernel/trace/bpf_trace.c       | 22 ++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  4 ++++
 tools/include/uapi/linux/bpf.h |  7 +++++++
 4 files changed, 40 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 162999b12790..6c96bf9c1f94 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3787,6 +3787,12 @@ union bpf_attr {
  *		*ARG_PTR_TO_BTF_ID* of type *task_struct*.
  *	Return
  *		Pointer to the current task.
+ *
+ * struct socket *bpf_sock_from_file(struct file *file)
+ *	Description
+ *		If the given file contains a socket, returns the associated socket.
+ *	Return
+ *		A pointer to a struct socket on success, or NULL on failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3948,6 +3954,7 @@ union bpf_attr {
 	FN(task_storage_get),		\
 	FN(task_storage_delete),	\
 	FN(get_current_task_btf),	\
+	FN(sock_from_file),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3530120fa280..d040d3ec8313 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1255,6 +1255,26 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_sock_from_file, struct file *, file)
+{
+	int err;
+
+	return (unsigned long) sock_from_file(file, &err);
+}
+
+BTF_ID_LIST(bpf_sock_from_file_btf_ids)
+BTF_ID(struct, socket)
+BTF_ID(struct, file)
+
+const struct bpf_func_proto bpf_sock_from_file_proto = {
+	.func		= bpf_sock_from_file,
+	.gpl_only	= true,
+	.ret_type	= RET_PTR_TO_BTF_ID_OR_NULL,
+	.ret_btf_id	= &bpf_sock_from_file_btf_ids[0],
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_sock_from_file_btf_ids[1],
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1349,6 +1369,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_per_cpu_ptr_proto;
 	case BPF_FUNC_bpf_this_cpu_ptr:
 		return &bpf_this_cpu_ptr_proto;
+	case BPF_FUNC_sock_from_file:
+		return &bpf_sock_from_file_proto;
 	default:
 		return NULL;
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 6769caae142f..99068ec40315 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -434,6 +434,8 @@ class PrinterHelpers(Printer):
             'struct xdp_md',
             'struct path',
             'struct btf_ptr',
+            'struct socket',
+            'struct file',
     ]
     known_types = {
             '...',
@@ -477,6 +479,8 @@ class PrinterHelpers(Printer):
             'struct task_struct',
             'struct path',
             'struct btf_ptr',
+            'struct socket',
+            'struct file',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 162999b12790..6c96bf9c1f94 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3787,6 +3787,12 @@ union bpf_attr {
  *		*ARG_PTR_TO_BTF_ID* of type *task_struct*.
  *	Return
  *		Pointer to the current task.
+ *
+ * struct socket *bpf_sock_from_file(struct file *file)
+ *	Description
+ *		If the given file contains a socket, returns the associated socket.
+ *	Return
+ *		A pointer to a struct socket on success, or NULL on failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3948,6 +3954,7 @@ union bpf_attr {
 	FN(task_storage_get),		\
 	FN(task_storage_delete),	\
 	FN(get_current_task_btf),	\
+	FN(sock_from_file),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.29.2.222.g5d2a92d10f8-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs)
  2020-11-12 20:09 [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs Florent Revest
@ 2020-11-12 20:28 ` Al Viro
  2020-11-13 18:25   ` Florent Revest
  2020-11-13  4:22 ` [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs kernel test robot
  2020-11-13  4:22 ` [RFC PATCH] bpf: bpf_sock_from_file_proto can be static kernel test robot
  2 siblings, 1 reply; 5+ messages in thread
From: Al Viro @ 2020-11-12 20:28 UTC (permalink / raw)
  To: Florent Revest
  Cc: bpf, ast, daniel, kafai, yhs, andrii, kpsingh, jackmanb,
	linux-kernel, Florent Revest, netdev

On Thu, Nov 12, 2020 at 09:09:44PM +0100, Florent Revest wrote:
> From: Florent Revest <revest@google.com>
> 
> eBPF programs can already check whether a file is a socket using
> file->f_op == &socket_file_ops but they can not convert file->private_data
> into a struct socket with BTF information. For that, we need a new
> helper that is essentially just a wrapper for sock_from_file.
> 
> sock_from_file can set an err value but this is only set to -ENOTSOCK
> when the return value is NULL so it's useless superfluous information.

That's a wrong way to handle that kind of stuff.  *IF* sock_from_file()
really has no need to return an error, its calling conventions ought to
be changed.  OTOH, if that is not the case, your API is a landmine.

That needs to be dealt with by netdev folks, rather than quietly papered
over in BPF code.

It does appear that there's no realistic cause to ever need other errors
there (well, short of some clown attaching a hook, pardon the obscenity),
so I would recommend something like the patch below (completely untested):

sanitize sock_from_file() calling conventions

deal with error value (always -ENOTSOCK) in the callers

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..07b33c1f34a9 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
 ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct seq_file *m = iocb->ki_filp->private_data;
-	size_t size = iov_iter_count(iter);
 	size_t copied = 0;
 	size_t n;
 	void *p;
@@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	}
 	/* if not empty - flush it first */
 	if (m->count) {
-		n = min(m->count, size);
-		if (copy_to_iter(m->buf + m->from, n, iter) != n)
-			goto Efault;
+		n = copy_to_iter(m->buf + m->from, m->count, iter);
 		m->count -= n;
 		m->from += n;
-		size -= n;
 		copied += n;
-		if (!size)
+		if (!iov_iter_count(iter) || m->count)
 			goto Done;
 	}
 	/* we need at least one record in buffer */
@@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	goto Done;
 Fill:
 	/* they want more? let's try to get some more */
+	/* m->count is positive and there's space left in iter */
 	while (1) {
 		size_t offs = m->count;
 		loff_t pos = m->index;
@@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			err = PTR_ERR(p);
 			break;
 		}
-		if (m->count >= size)
+		if (m->count >= iov_iter_count(iter))
 			break;
 		err = m->op->show(m, p);
 		if (seq_has_overflowed(m) || err) {
@@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		}
 	}
 	m->op->stop(m, p);
-	n = min(m->count, size);
-	if (copy_to_iter(m->buf, n, iter) != n)
-		goto Efault;
+	n = copy_to_iter(m->buf, m->count, iter);
 	copied += n;
 	m->count -= n;
 	m->from = n;
 Done:
-	if (!copied)
-		copied = err;
-	else {
+	if (unlikely(!copied)) {
+		copied = m->count ? -EFAULT : err;
+	} else {
 		iocb->ki_pos += copied;
 		m->read_pos += copied;
 	}
@@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 Enomem:
 	err = -ENOMEM;
 	goto Done;
-Efault:
-	err = -EFAULT;
-	goto Done;
 }
 EXPORT_SYMBOL(seq_read_iter);
 

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs
  2020-11-12 20:09 [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs Florent Revest
  2020-11-12 20:28 ` saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs) Al Viro
@ 2020-11-13  4:22 ` kernel test robot
  2020-11-13  4:22 ` [RFC PATCH] bpf: bpf_sock_from_file_proto can be static kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2020-11-13  4:22 UTC (permalink / raw)
  To: Florent Revest, bpf
  Cc: kbuild-all, ast, daniel, kafai, yhs, andrii, kpsingh, jackmanb,
	linux-kernel, Florent Revest

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Hi Florent,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on next-20201112]
[cannot apply to bpf/master linus/master v5.10-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Expose-a-bpf_sock_from_file-helper-to-tracing-programs/20201113-041233
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s001-20201111 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-107-gaf3512a6-dirty
        # https://github.com/0day-ci/linux/commit/7fd0725829bee29729e56964e32cad7eb9dc9a85
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Expose-a-bpf_sock_from_file-helper-to-tracing-programs/20201113-041233
        git checkout 7fd0725829bee29729e56964e32cad7eb9dc9a85
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> kernel/trace/bpf_trace.c:1267:29: sparse: sparse: symbol 'bpf_sock_from_file_proto' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32312 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC PATCH] bpf: bpf_sock_from_file_proto can be static
  2020-11-12 20:09 [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs Florent Revest
  2020-11-12 20:28 ` saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs) Al Viro
  2020-11-13  4:22 ` [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs kernel test robot
@ 2020-11-13  4:22 ` kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2020-11-13  4:22 UTC (permalink / raw)
  To: Florent Revest, bpf
  Cc: kbuild-all, ast, daniel, kafai, yhs, andrii, kpsingh, jackmanb,
	linux-kernel, Florent Revest


Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 bpf_trace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4749575b81b2d1..c34c81095d61c1 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1264,7 +1264,7 @@ BTF_ID_LIST(bpf_sock_from_file_btf_ids)
 BTF_ID(struct, socket)
 BTF_ID(struct, file)
 
-const struct bpf_func_proto bpf_sock_from_file_proto = {
+static const struct bpf_func_proto bpf_sock_from_file_proto = {
 	.func		= bpf_sock_from_file,
 	.gpl_only	= true,
 	.ret_type	= RET_PTR_TO_BTF_ID_OR_NULL,

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs)
  2020-11-12 20:28 ` saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs) Al Viro
@ 2020-11-13 18:25   ` Florent Revest
  0 siblings, 0 replies; 5+ messages in thread
From: Florent Revest @ 2020-11-13 18:25 UTC (permalink / raw)
  To: Al Viro
  Cc: bpf, ast, daniel, kafai, yhs, andrii, kpsingh, jackmanb,
	linux-kernel, Florent Revest, netdev

On Thu, 2020-11-12 at 20:28 +0000, Al Viro wrote:
> On Thu, Nov 12, 2020 at 09:09:44PM +0100, Florent Revest wrote:
> > From: Florent Revest <revest@google.com>
> > 
> > eBPF programs can already check whether a file is a socket using
> > file->f_op == &socket_file_ops but they can not convert file-
> > >private_data into a struct socket with BTF information. For that,
> > we need a new helper that is essentially just a wrapper for
> > sock_from_file.
> > 
> > sock_from_file can set an err value but this is only set to
> > -ENOTSOCK when the return value is NULL so it's useless superfluous
> > information.
> 
> That's a wrong way to handle that kind of stuff.  *IF*
> sock_from_file() really has no need to return an error, its calling
> conventions ought to be changed. OTOH, if that is not the case, your
> API is a landmine.
> 
> That needs to be dealt with by netdev folks, rather than quietly
> papered over in BPF code.

Sounds good to me. :) What do netdev folks think of this ?

> It does appear that there's no realistic cause to ever need other
> errors there (well, short of some clown attaching a hook, pardon the
> obscenity), so I would recommend something like the patch below
> (completely untested):

Thanks for taking the time but is this the patch you meant to send?

> sanitize sock_from_file() calling conventions
> 
> deal with error value (always -ENOTSOCK) in the callers
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..07b33c1f34a9 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
>  ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  {
>  	struct seq_file *m = iocb->ki_filp->private_data;
> -	size_t size = iov_iter_count(iter);
>  	size_t copied = 0;
>  	size_t n;
>  	void *p;
> @@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb,
> struct iov_iter *iter)
>  	}
>  	/* if not empty - flush it first */
>  	if (m->count) {
> -		n = min(m->count, size);
> -		if (copy_to_iter(m->buf + m->from, n, iter) != n)
> -			goto Efault;
> +		n = copy_to_iter(m->buf + m->from, m->count, iter);
>  		m->count -= n;
>  		m->from += n;
> -		size -= n;
>  		copied += n;
> -		if (!size)
> +		if (!iov_iter_count(iter) || m->count)
>  			goto Done;
>  	}
>  	/* we need at least one record in buffer */
> @@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
>  	goto Done;
>  Fill:
>  	/* they want more? let's try to get some more */
> +	/* m->count is positive and there's space left in iter */
>  	while (1) {
>  		size_t offs = m->count;
>  		loff_t pos = m->index;
> @@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
>  			err = PTR_ERR(p);
>  			break;
>  		}
> -		if (m->count >= size)
> +		if (m->count >= iov_iter_count(iter))
>  			break;
>  		err = m->op->show(m, p);
>  		if (seq_has_overflowed(m) || err) {
> @@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb,
> struct iov_iter *iter)
>  		}
>  	}
>  	m->op->stop(m, p);
> -	n = min(m->count, size);
> -	if (copy_to_iter(m->buf, n, iter) != n)
> -		goto Efault;
> +	n = copy_to_iter(m->buf, m->count, iter);
>  	copied += n;
>  	m->count -= n;
>  	m->from = n;
>  Done:
> -	if (!copied)
> -		copied = err;
> -	else {
> +	if (unlikely(!copied)) {
> +		copied = m->count ? -EFAULT : err;
> +	} else {
>  		iocb->ki_pos += copied;
>  		m->read_pos += copied;
>  	}
> @@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct
> iov_iter *iter)
>  Enomem:
>  	err = -ENOMEM;
>  	goto Done;
> -Efault:
> -	err = -EFAULT;
> -	goto Done;
>  }
>  EXPORT_SYMBOL(seq_read_iter);


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-13 18:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 20:09 [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs Florent Revest
2020-11-12 20:28 ` saner sock_from_file() calling conventions (was Re: [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs) Al Viro
2020-11-13 18:25   ` Florent Revest
2020-11-13  4:22 ` [PATCH] bpf: Expose a bpf_sock_from_file helper to tracing programs kernel test robot
2020-11-13  4:22 ` [RFC PATCH] bpf: bpf_sock_from_file_proto can be static kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).