Date: Wed, 12 Apr 2017 00:28:42 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Dave Jones <davej@codemonkey.org.uk>,
        Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: iov_iter_pipe warning.
Message-ID: <20170411232842.GI29622@ZenIV.linux.org.uk>
References: <20170410192800.GC29622@ZenIV.linux.org.uk>
 <20170410194206.loesu5licstif7or@codemonkey.org.uk>
 <20170410195711.GD29622@ZenIV.linux.org.uk>
 <20170410234830.tmqdhpjtfdveor3c@codemonkey.org.uk>
 <20170411002215.GE29622@ZenIV.linux.org.uk>
 <20170411030532.vcam25fz6224ny2h@codemonkey.org.uk>
 <20170411032839.GF29622@ZenIV.linux.org.uk>
 <20170411205336.uyz5vfw52twhh6ob@codemonkey.org.uk>
 <20170411211216.GH29622@ZenIV.linux.org.uk>
 <20170411222502.ldgahltwvrrxdbbw@codemonkey.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170411222502.ldgahltwvrrxdbbw@codemonkey.org.uk>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3244
Lines: 94

On Tue, Apr 11, 2017 at 06:25:02PM -0400, Dave Jones wrote:

> ffffffff812b3130 T generic_splice_sendpage
> 
> This one spat out all by itself.

No need to print ->f_op for that one - can be only socket_file_ops.  Now,
the address family of that socket would be interesting...

How about adding to that printk (under if (WARN_ON()) something like
	file = sd->u.file;
	if (file->f_op->splice_write == generic_splice_sendpage) {
		struct socket *sock = file->private_data;
		printk(KERN_ERR "socket [%d, %p]\n", sock->type, sock->ops);
	}
	printk(KERN_ERR "in->f_op = %p\n", in->f_op);

Said that, we seem to have
	* a pipe with some amount of data in it
	* generic_splice_sendpage() called on that pipe, with len equal to
the amount in the pipe.  Hopefully.
	* generic_splice_sendpage() returning the value equal to len...
	* ... and not draining the pipe entirely.

generic_splice_sendpage() is calling this:

ssize_t __splice_from_pipe(struct pipe_inode_info *pipe, struct splice_desc *sd,
                           splice_actor *actor)
{
        int ret;

        splice_from_pipe_begin(sd); 
        do {
                cond_resched();
                ret = splice_from_pipe_next(pipe, sd);
                if (ret > 0)
                        ret = splice_from_pipe_feed(pipe, sd, actor);
        } while (ret > 0);
        splice_from_pipe_end(pipe, sd);

        return sd->num_spliced ? sd->num_spliced : ret;
}

It has returned a positive number.  That must have been sd->num_spliced.
splice_from_pipe_begin() sets it to 0 and the only place where it is
updated is
                ret = actor(pipe, buf, sd);
                if (ret <= 0)
                        return ret;

                buf->offset += ret;
                buf->len -= ret;

                sd->num_spliced += ret;
                sd->len -= ret;
                sd->pos += ret;
                sd->total_len -= ret;

                if (!buf->len) {
                        pipe_buf_release(pipe, buf);
                        pipe->curbuf = (pipe->curbuf + 1) & (pipe->buffers - 1);
                        pipe->nrbufs--;

in splice_from_pipe_feed().  Whatever actor() is doing, the amount we
drain from the pipe is equal to the amount we add to ->num_spliced.

In other words, sending part looks reasonably solid.  Another thing that
might have happened is
                ret = do_splice_to(in, &pos, pipe, len, flags);
                if (unlikely(ret <= 0))
                        goto out_release;
returning less than it has actually dumped into the pipe in some situations.

Which means default_file_splice_read() called on an empty pipe and
returning less than it has put there.  The thing is, the last thing
that function does is
        iov_iter_advance(&to, copied);  /* truncates and discards */
	return res;
and we would have to have copied > res > 0 for that scenario to happen...

Interesting...  How about
	if (res > 0 && pipe == current->splice_pipe) {
		int idx = pipe->curbuf;
		int n = pipe->nrbufs;
		size_t size = 0;
		while (n--) {
			size += pipe->bufs[idx++].len;
			if (idx == pipe->buffers)
				idx = 0;
		}
		WARN_ON(len != res);
	}
just before the return in default_file_splice_read()?  WARN_ON_ONCE,
perhaps, to avoid cascades...