RE: [PATCH v2] ceph: Fix kernel crash in generic/397 test

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

Hi Ilya,

On Fri, 2025-01-17 at 13:06 +0100, Ilya Dryomov wrote:
> On Fri, Jan 17, 2025 at 4:51 AM Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> > 
> > 

<skipped>

> > Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > ---
> >  fs/ceph/addr.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 85936f6d2bf7..5e6ba92219f3 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -396,6 +396,15 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
> >                 struct page **pages;
> >                 size_t page_off;
> > 
> > +               /*
> > +                * The io_iter.count needs to be corrected to aligned length.
> > +                * Otherwise, iov_iter_get_pages_alloc2() operates with
> > +                * the initial unaligned length value. As a result,
> > +                * ceph_msg_data_cursor_init() triggers BUG_ON() in the case
> > +                * if msg->sparse_read_total > msg->data_length.
> > +                */
> > +               subreq->io_iter.count = len;
> 
> Hi Slava,
> 
> So I take it that my hunch that it's subreq->io_iter and commenting out
> "len = err" assignment worked?  TBH munging the count this way feels as
> much of an ugly workaround as ignoring iov_iter_get_pages_alloc2()
> return value (unless it's an error) to me.
> 

As far as I can see, the main issue that subreq->io_iter keeps
unaligned value in subreq->io_iter.count. And if the alignment
was made earlier [1]:

	len = subreq->len;
	ceph_fscrypt_adjust_off_and_len(inode, &off, &len);

then it needs to correct the value of subreq->io_iter.count later
before calling iov_iter_get_pages_alloc2() [2]:

	if (IS_ENCRYPTED(inode)) {
		struct page **pages;
		size_t page_off;

		err = iov_iter_get_pages_alloc2(&subreq->io_iter, &pages, len, &page_off);
		if (err < 0) {
			doutc(cl, "%llx.%llx failed to allocate pages, %d\n",
			      ceph_vinop(inode), err);
			goto out;
		}

		/* should always give us a page-aligned read */
		WARN_ON_ONCE(page_off);
		len = err;
		err = 0;

		osd_req_op_extent_osd_data_pages(req, 0, pages, len, 0, false,
						 false);
	}

or ceph_netfs_issue_read() needs to be called with already
aligned value of subreq->io_iter.count.

If we have correct value (aligned) of subreq->io_iter.count, then
it doesn't matter "len = err" commented or not.

> Since this confirms that we have a regression introduced in ee4cdf7ba857
> ("netfs: Speed up buffered reading"), let's wait for David to chime in.
> 

Sounds good!

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.13-rc3/source/fs/ceph/addr.c#L365
[2] https://elixir.bootlin.com/linux/v6.13-rc3/source/fs/ceph/addr.c#L395