Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700152 > unrolled thread

Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Colin Walters <walters@verbum.org> - 2017-07-31 18:10 +0200
    Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Colin Walters <walters@verbum.org> - 2017-07-31 18:40 +0200
      Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Colin Walters <walters@verbum.org> - 2017-07-31 19:50 +0200
        Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-07-31 20:30 +0200
          Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Colin Walters <walters@verbum.org> - 2017-08-01 04:20 +0200
            Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Dave Chinner <david@fromorbit.com> - 2017-08-01 04:50 +0200
              Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Christoph Hellwig <hch@lst.de> - 2017-08-05 11:50 +0200
    Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE Dan Williams <dan.j.williams@intel.com> - 2017-07-31 18:40 +0200

#1700152 — Re: [PATCH 1/3] fs, xfs: introduce S_IOMAP_IMMUTABLE

On Sat, Jul 29, 2017, at 03:43 PM, Dan Williams wrote:
> An inode with this flag set indicates that the file's block map cannot
> be changed, no size change, deletion, hole-punch, range collapse, or
> reflink.
> 
> The implementation of toggling the flag and sealing the state of the
> extent map is saved for a later patch. The functionality provided by
> S_IOMAP_IMMUTABLE, once toggle support is added, will be a superset of
> that provided by S_SWAPFILE, and it is targeted to replace it.
> 
> For now, only xfs and the core vfs are updated to consider the new flag.

Quite a while ago I started a request for O_OBJECT:
http://www.spinics.net/lists/linux-fsdevel/msg75085.html
A few months ago I was thinking about that more and realized
it'd likely be more palatable to land as an inode flag, like
you're doing here.

Now, S_IOMAP_IMMUTABLE would be quite close to the semantics
I want for ostree, except we also want to disallow
changes to the inode uid, gid or mode.  (Extended attributes are 
a whole other story; but I'd like to at least disallow changes to the
security. namespace).

The goal here is mostly about resilience to *accidental* changes;
think an admin doing `cp /path/to/binary /usr/bin/bash` which
does open(O_TRUNC), which would hence corrupt all hardlinks.

S_IOMAP_IMMUTABLE would give a lot of great protection against
those types of accidental changes - most of them are either going
to be open(O_TRUNC) or O_APPEND.   Since you're touching various
write paths here, perhaps we can also add
S_CONTENTS_IMMUTABLE or something at the same time?

If this lands as is - I'm quite likely to change ostree to use it;
any objections to that?  As mentioned in the thread, there are several
other cases of "content immutable" files in userspace, such as
QEMU "qcow2", git objects.  And really the most classic example is
/etc/sudoers and the need for a special "visudo" program to really
ensure that editors don't do in-place overwrites.

But it'd be great if we can use this push to also land "content immutabilty"
or however we decide to call it.

[toc] | [next] | [standalone]

#1700184

On Mon, Jul 31, 2017, at 12:29 PM, Dan Williams wrote:
> 
> How is S_CONTENTS_IMMUTABLE different than S_IMMUTABLE?

We still want the ability to make hardlinks.

[toc] | [prev] | [next] | [standalone]

#1700262

On Mon, Jul 31, 2017, at 12:32 PM, Colin Walters wrote:
> On Mon, Jul 31, 2017, at 12:29 PM, Dan Williams wrote:
> > 
> > How is S_CONTENTS_IMMUTABLE different than S_IMMUTABLE?
> 
> We still want the ability to make hardlinks.

Also of course, symmetrically, to unlink.   If we used S_IMMUTABLE for /etc/sudoers,
it'd still be racy since one would have to transiently remove the flag in order
to replace it with a new version.

Related to this topic is the fact that S_IMMUTABLE is itself mutable; I
think once S_IMMUTABLE_CONTENTS is set, it would not be able to made
mutable again.  

Also I just remembered that since then memfd_create() and more notably
fcntl(F_ADD_SEALS) landed - in fact it already has flags for what we want
here AFAICS.  Your S_IOMAP_IMMUTABLE is fcntl(F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW)
and mine just adds in F_SEAL_WRITE.  I think there was some discussion
of the seals for persistent files when memfd_create() landed, but I can't
find it offhand.

[toc] | [prev] | [next] | [standalone]

#1700308

On Mon, Jul 31, 2017 at 01:42:13PM -0400, Colin Walters wrote:
> 
> 
> On Mon, Jul 31, 2017, at 12:32 PM, Colin Walters wrote:
> > On Mon, Jul 31, 2017, at 12:29 PM, Dan Williams wrote:
> > > 
> > > How is S_CONTENTS_IMMUTABLE different than S_IMMUTABLE?
> > 
> > We still want the ability to make hardlinks.
> 
> Also of course, symmetrically, to unlink.   If we used S_IMMUTABLE for /etc/sudoers,
> it'd still be racy since one would have to transiently remove the flag in order
> to replace it with a new version.
> 
> Related to this topic is the fact that S_IMMUTABLE is itself mutable; I
> think once S_IMMUTABLE_CONTENTS is set, it would not be able to made
> mutable again.  
> 
> Also I just remembered that since then memfd_create() and more notably
> fcntl(F_ADD_SEALS) landed - in fact it already has flags for what we want
> here AFAICS.  Your S_IOMAP_IMMUTABLE is fcntl(F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW)

I don't think F_SEAL_{SHRINK,GROW} prevents reflinking or CoW of file data,
which are two things that cannot happen under S_IOMAP_IMMUTABLE that
aren't size changes.  From the implementation it looks like shrink and
grow are only supposed to disallow changes to i_size, not i_blocks (or
the file block map).

Then again, I suppose F_SEAL_* only work on shmem, so maybe it simply
isn't defined for any other filesystem...?  e.g. it doesn't prohibit
reflink, but the only fs implementing seals doesn't support reflink.

<shrug>

Seals cannot be removed, which is too strict for the S_IOMAP_IMMUTABLE
user cases being presented.

> and mine just adds in F_SEAL_WRITE.  I think there was some discussion
> of the seals for persistent files when memfd_create() landed, but I can't
> find it offhand.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1700585

On Mon, Jul 31, 2017, at 02:23 PM, Darrick J. Wong wrote:

> I don't think F_SEAL_{SHRINK,GROW} prevents reflinking or CoW of file data,
> which are two things that cannot happen under S_IOMAP_IMMUTABLE that
> aren't size changes.  From the implementation it looks like shrink and
> grow are only supposed to disallow changes to i_size, not i_blocks (or
> the file block map).

True. 

> Then again, I suppose F_SEAL_* only work on shmem, so maybe it simply
> isn't defined for any other filesystem...?  e.g. it doesn't prohibit
> reflink, but the only fs implementing seals doesn't support reflink.
> 
> <shrug>
> 
> Seals cannot be removed, which is too strict for the S_IOMAP_IMMUTABLE
> user cases being presented.

To be clear, the set of use cases is swap files and DAX, right?  Or is there anything else?
I can't imagine why anyone would want to turn a swap file back into a regular file.
I haven't fully followed DAX, but I'd take your word for it if people want to
be able to remove the flag after.

Anyways, I think your broader point is right; the use cases are different enough
that it doesn't make sense to try to add S_CONTENT_IMMUTABLE (or however 
one decides to call it) at the same time.

[toc] | [prev] | [next] | [standalone]

#1700594

On Mon, Jul 31, 2017 at 10:15:12PM -0400, Colin Walters wrote:
> On Mon, Jul 31, 2017, at 02:23 PM, Darrick J. Wong wrote:
> 
> > I don't think F_SEAL_{SHRINK,GROW} prevents reflinking or CoW of file data,
> > which are two things that cannot happen under S_IOMAP_IMMUTABLE that
> > aren't size changes.  From the implementation it looks like shrink and
> > grow are only supposed to disallow changes to i_size, not i_blocks (or
> > the file block map).
> 
> True. 
> 
> > Then again, I suppose F_SEAL_* only work on shmem, so maybe it simply
> > isn't defined for any other filesystem...?  e.g. it doesn't prohibit
> > reflink, but the only fs implementing seals doesn't support reflink.
> > 
> > <shrug>
> > 
> > Seals cannot be removed, which is too strict for the S_IOMAP_IMMUTABLE
> > user cases being presented.
> 
> To be clear, the set of use cases is swap files and DAX, right?  Or is there anything else?

I've outlined other use cases in previous discussions. To repeat
myself, every so often we get someone with, say, a new high
speed camera that want to dma the camera frames direct to the
storage because they can't push 500,000 frames/s through the CPU
to storage. Hence they want to bypass the OS and DMA the data direct
to the storage. To do this they need a mechanism to freeze and unfreeze
the block map of the file so that nothing modifies the block map
while the camera hardware is dumping data direct to the storage.
Immutable extent maps provide the functionality they need to
implement this safely.

There's also other similar use cases for RDMA targets on PMEM
(regardless of whether DAX is enabled or not), and I've come across
a couple of requests for mechanisms to allow fabric based nvme
storage to do direct data transfers between storage devices, too.
All of these use cases can be safely implemented if there is a
mechanism to mark extent maps as immutable for the duration of
the operation they need to perform.

> I can't imagine why anyone would want to turn a swap file back into a regular file.
> I haven't fully followed DAX, but I'd take your word for it if people want to
> be able to remove the flag after.

DAX isn't the driver of that functionality, it's the other use cases
that need it, and why the proposed "only remove flag if len == 0"
API is a non-starter....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1704611

On Tue, Aug 01, 2017 at 12:42:18PM +1000, Dave Chinner wrote:
> I've outlined other use cases in previous discussions. To repeat
> myself, every so often we get someone with, say, a new high
> speed camera that want to dma the camera frames direct to the
> storage because they can't push 500,000 frames/s through the CPU
> to storage. Hence they want to bypass the OS and DMA the data direct
> to the storage. To do this they need a mechanism to freeze and unfreeze
> the block map of the file so that nothing modifies the block map
> while the camera hardware is dumping data direct to the storage.
> Immutable extent maps provide the functionality they need to
> implement this safely.

And we have such a mechanism already: it's called the iolock during
I/O, and dirct I/O.  I've worked on plenty such schemes and the proper way
works perfectly fine.  Just because people ask for stupid ways to
archives that doesn't mean they understand what they are doing.

> There's also other similar use cases for RDMA targets on PMEM
> (regardless of whether DAX is enabled or not), and I've come across
> a couple of requests for mechanisms to allow fabric based nvme
> storage to do direct data transfers between storage devices, too.
> All of these use cases can be safely implemented if there is a
> mechanism to mark extent maps as immutable for the duration of
> the operation they need to perform.

As someone who spent most of them time on the last 2 years in this
area: we have a massive problem discoverability and addressing
(lack of struct page) for p2p devices.  We have absolutely no problem
with the direct I/O model with them.

> DAX isn't the driver of that functionality, it's the other use cases
> that need it, and why the proposed "only remove flag if len == 0"
> API is a non-starter....

The other "use" cases are even more bullshit than the DAX one.

[toc] | [prev] | [next] | [standalone]

#1700185

On Mon, Jul 31, 2017 at 9:02 AM, Colin Walters <walters@verbum.org> wrote:
> On Sat, Jul 29, 2017, at 03:43 PM, Dan Williams wrote:
>> An inode with this flag set indicates that the file's block map cannot
>> be changed, no size change, deletion, hole-punch, range collapse, or
>> reflink.
>>
>> The implementation of toggling the flag and sealing the state of the
>> extent map is saved for a later patch. The functionality provided by
>> S_IOMAP_IMMUTABLE, once toggle support is added, will be a superset of
>> that provided by S_SWAPFILE, and it is targeted to replace it.
>>
>> For now, only xfs and the core vfs are updated to consider the new flag.
>
> Quite a while ago I started a request for O_OBJECT:
> http://www.spinics.net/lists/linux-fsdevel/msg75085.html
> A few months ago I was thinking about that more and realized
> it'd likely be more palatable to land as an inode flag, like
> you're doing here.
>
> Now, S_IOMAP_IMMUTABLE would be quite close to the semantics
> I want for ostree, except we also want to disallow
> changes to the inode uid, gid or mode.  (Extended attributes are
> a whole other story; but I'd like to at least disallow changes to the
> security. namespace).
>
> The goal here is mostly about resilience to *accidental* changes;
> think an admin doing `cp /path/to/binary /usr/bin/bash` which
> does open(O_TRUNC), which would hence corrupt all hardlinks.
>
> S_IOMAP_IMMUTABLE would give a lot of great protection against
> those types of accidental changes - most of them are either going
> to be open(O_TRUNC) or O_APPEND.   Since you're touching various
> write paths here, perhaps we can also add
> S_CONTENTS_IMMUTABLE or something at the same time?
>
> If this lands as is - I'm quite likely to change ostree to use it;
> any objections to that?  As mentioned in the thread, there are several
> other cases of "content immutable" files in userspace, such as
> QEMU "qcow2", git objects.  And really the most classic example is
> /etc/sudoers and the need for a special "visudo" program to really
> ensure that editors don't do in-place overwrites.
>
> But it'd be great if we can use this push to also land "content immutabilty"
> or however we decide to call it.

How is S_CONTENTS_IMMUTABLE different than S_IMMUTABLE?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web