Re: [PATCH 1/8] mm/fs: don't allow writes to immutable files

Darrick J. Wong
2019-06-11T03:26:20Z

On Mon, Jun 11, 2019 at 04:41:54PM -0400, Theodore Ts'o wrote:
> On Mon, Jun 10, 2019 at 09:09:34AM -0700, Darrick J. Wong wrote:
> > > I was planning on only taking 8/8 through the ext4 tree. I also added
> > > a patch which filtered writes, truncates, and page_mkwrites (but not
> > > mmap) for immutable files at the ext4 level.
> >
> > *Oh*. I saw your reply attached to the 1/8 patch and thought that was
> > the one you were taking. I was sort of surprised, tbh. :)
>
> Sorry, my bad. I mis-replied to the wrong e-mail message :-)
>
> > > I *could* take this patch through the mm/fs tree, but I wasn't sure
> > > what your plans were for the rest of the patch series, and it seemed
> > > like it hadn't gotten much review/attention from other fs or mm folks
> > > (well, I guess Brian Foster weighed in).
> >
> > > What do you think?
> >
> > Not sure. The comments attached to the LWN story were sort of nasty,
> > and now that a couple of people said "Oh, well, Debian documented the
> > inconsistent behavior so just let it be" I haven't felt like
> > resurrecting the series for 5.3.
>
> Ah, I had missed the LWN article. <Looks>
>
> Yeah, it's the same set of issues that we had discussed when this
> first came up. We can go round and round on this one; It's true that
> root can now cause random programs which have a file mmap'ed for
> writing to seg fault, but root has a million ways of killing and
> otherwise harming running application programs, and it's unlikely
> files get marked for immutable all that often. We just have to pick
> one way of doing things, and let it be same across all the file
> systems.
>
> My understanding was that XFS had chosen to make the inode immutable
> as soon as the flag is set (as opposed to forbidding new fd's to be
> opened which were writeable), and I was OK moving ext4 to that common
> interpretation of the immmutable bit, even though it would be a change
> to ext4.

<nod> It started as "just do this to xfs" and has now become a vfs level
change...

> And then when I saw that Amir had included a patch that would cause
> test failures unless that patch series was applied, it seemed that we
> had all thought that the change was a done deal. Perhaps we should
> have had a more explicit discussion when the test was sent for review,
> but I had assumed it was exclusively a copy_file_range set of tests,
> so I didn't realize it was going to cause ext4 failures.

And here we see the inconsistent behavior causing developer confusion. :)

I think Amir's c_f_r tests just check the existing behavior (of just
c_f_r) that you can't (most of the time) copy into a file that you
opened for write but that the administrator has since marked immutable.

/That/ behavior in turn came from the original implementation that would
try reflink which would fail on the immutable destination check and then
fail the whole call ... I think?

--D

> - Ted