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 <rev...@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 <v...@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);
