Re: [PATCH] fuse: fix illegal access to inode with reused nodeid

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Jun 17, 2021 at 10:52 AM Nikolaus Rath <Nikolaus@rath.org> wrote:
>
> On Jun 16 2021, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Jun 16, 2021 at 8:25 PM Nikolaus Rath <nikolaus@rath.org> wrote:
> >>
> >> Hi Amir,
> >>
> >> On Wed, 16 Jun 2021, at 16:03, Amir Goldstein wrote:
> >> > Per request from Nikolaus, I modified the passthrough_hp example
> >> > to reuse inodes on last close+unlink, so it now hits the failure in the
> >> > new test with upstream kernel and it passes the test with this kernel fix.
> >> >
> >> > Thanks,
> >> > Amir.
> >> >
> >> > [2] https://github.com/libfuse/libfuse/pull/612
> >>
> >> Actually, I am no longer sure this was a good idea. Having the libfuse test suite detect
> >> problems that with the kernel doesn't seem to helpful.. I think the testsuite should
> >> identify problems in libfuse.  Currently, having the tests means that users might be
> >> hesitant to update to the newer libfuse because of the failing test - when in fact there
> >> is nothing wrong with libfuse at all.
> >>
> >
> > I suppose you are right.
> > I could take the tesy_syscalls test to xfstest, but fuse support for
> > xfstests is still WIP.
> >
> >> I assume the test will start failing on some future kernel (which is why it passed CL),
> >> and then start passing again for some kernel after that?
> >
> > I was not aware that it passes CI.
> > There are no test results available on github.
>
> Arg. Looks like something is broken there. I mistook the absence of
> results for a passing result.
>
> > I am not aware of any specific kernel version where the test should pass,
> > but the results also depend on the underlying filesystem.
> >
> > If your underlying filesystem is btrfs, it does not reuse inode numbers
> > at all, so the test will not fail.
> >
> > For me the test fails on ext4 and xfs on LTS kernel 5.10.
> > As I wrote in PR:
> > "...Fails the modified test_syscalls in this PR on upstream kernel"
> >
> > If you revert the last commit the test would pass on upstream kernel:
> > 80f2b8b ("passthrough_hp: excercise reusing inode numbers")
> >
> > We could make behavior of passthrough_hp example depend
> > on some minimal kernel protocol version or new kernel capability like
> > FUSE_SETXATTR_EXT if Miklos intends to merge the fix for the coming
> > kernel release or we could just make that new test optional via pytest option.
> >
> > After all, regardless of the kernel bug, this adds test coverage that was
> > missing, so it also covers a possible future regression in libfuse.
> >
> > Let me know if you want me to implement any of the listed options.
>
> I don't want an old kernel to result in libfuse unit tests failing, but
> I think it's a good idea to cover this case in some form.
>
> Would you be able to make the test conditional on a recent enough kernel
> version?
>

That looks trivial, like:
def test_write_cache(tmpdir, writeback, output_checker):
    if writeback and LooseVersion(platform.release()) < '3.14':
        pytest.skip('Requires kernel 3.14 or newer')

I'll just wait to see if Miklos takes the kernel fix to v5.13
so I know which version to use in the condition.

Thanks,
Amir.