Linux-mtd Archive on
 help / color / Atom feed
From: Eric Biggers <>
To: "Theodore Y. Ts'o" <>
Cc: Satya Tangirala <>,,,,,,,,,
	Paul Crowley <>
Subject: Re: [f2fs-dev] [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl
Date: Wed, 31 Jul 2019 18:11:40 -0700
Message-ID: <20190801011140.GB687@sol.localdomain> (raw)
In-Reply-To: <>

On Wed, Jul 31, 2019 at 07:38:43PM -0400, Theodore Y. Ts'o wrote:
> On Wed, Jul 31, 2019 at 11:38:02AM -0700, Eric Biggers wrote:
> > 
> > This is perhaps different from what users expect from unlink().  It's well known
> > that unlink() just deletes the filename, not the file itself if it's still open
> > or has other links.  And unlink() by itself isn't meant for use cases where the
> > file absolutely must be securely erased.  But FS_IOC_REMOVE_ENCRYPTION_KEY
> > really is meant primarily for that sort of thing.
> Seems to me that part of the confusion is FS_IOC_REMOVE_ENCRYPTION_KEY
> does two things.  One is "remove the user's handle on the key".  The
> other is "purge all keys" (which requires root).  So it does two
> different things with one ioctl.

Well, it's either

1a. Remove the user's handle.
1b. Remove all users' handles.  (FSCRYPT_REMOVE_KEY_FLAG_ALL_USERS)


2. If no handles remain, try to evict all inodes that use the key.

By "purge all keys" do you mean step (2)?  Note that it doesn't require root by
itself; root is only required to remove other users' handles (1b).

It could be argued that (2) should be a separate ioctl, so we'd have UNLINK_KEY
then LOCK_KEY.  But is there a real use case for this split?  I.e. would anyone
ever want to UNLINK_KEY without also LOCK_KEY?  Is that really something we
want/need to support?  I'd really like the API to be as straightforward as
possible for the normal use case of locking a directory, and not require some
series of multiple ioctl's, which would be more difficult to use correctly.

> > To give a concrete example: my patch for the userspace tool
> > adds a command 'fscrypt lock' which locks an
> > encrypted directory.  If, say, someone runs 'fscrypt unlock' as uid 0 and then
> > 'fscrypt lock' as uid 1000, then FS_IOC_REMOVE_ENCRYPTION_KEY can't actually
> > remove the key.  I need to make the tool show a proper error message in this
> > case.  To do so, it would help to get a unique error code (e.g. EUSERS) from
> > FS_IOC_REMOVE_ENCRYPTION_KEY, rather than get the ambiguous error code ENOKEY
> > and have to call FS_IOC_GET_ENCRYPTION_KEY_STATUS to get the real status.
> What about having "fscrypt lock" call FS_IOC_GET_ENCRYPTION_KEY_STATUS
> and print a warning message saying, "we can't lock it because N other
> users who have registered a key".  I'd argue fscrypt should do this
> regardless of whether or not FS_IOC_REMOVE_ENCRYPTION_KEY returns
> EUSERS or not.

Shouldn't "fscrypt lock" still remove the user's handle, as opposed to refuse to
do anything, though?  So it would still need to call
FS_IOC_REMOVE_ENCRYPTION_KEY, and could get the status from it rather than also

Though, FS_IOC_GET_ENCRYPTION_KEY_STATUS would be needed if we wanted to show
the specific count of other users.

> > Also, we already have the EBUSY case.  This means that the ioctl removed the
> > master key secret itself; however, some files were still in-use, so the key
> > remains in the "incompletely removed" state.  If we were actually going for
> > unlink() semantics, then for consistency this case really ought to return 0 and
> > unlink the key object, and people who care about in-use files would need to use
> > FS_IOC_GET_ENCRYPTION_KEY_STATUS.  But most people *will* care about this, and
> > may even want to retry the ioctl later, which isn't something youh can do with
> > pure unlink() semantics.
> It seems to me that the EBUSY and EUSERS errors should be status bits
> which gets returned to the user in a bitfield --- and if the key has
> been removed, or the user's claim on the key's existence has been
> removed, the ioctl returns success.
> That way we don't have to deal with the semantic disconnect where some
> errors don't actually change system state, and other errors that *do*
> change system state (as in, the key gets removed, or the user's claim
> on the key gets removed), but still returns than error.
> We could also add a flag which indicates where if there are files that
> are still busy, or there are other users keeping a key in use, the
> ioctl fails hard and returns an error.  At least that way we keep
> consistency where an error means, "nothing has changed".
> 	    	     	   	  	   - Ted

Do you mean use a positive return value, or do you mean add an output field to
the struct passed to the ioctl?

The latter might be more error-prone, since it invites bugs where a directory
silently fails to be locked, because the second field was not checked.

Either way note that it doesn't really need to be a bitfield, since you can't
have both statuses at the same time.  I.e. if there are still other users, we
couldn't have even gotten to checking for in-use files.

> P.S.  BTW, one of the comments which I didn't make was the
> documentation didn't adequately explain which error codes means,
> "success but with a caveat", and which errors means, "we failed and
> didn't do anything".  But since I was arguing for changing the
> behavior, I decided not to complain about the documentation.

Yes, in any case the FS_IOC_REMOVE_ENCRYPTION_KEY documentation needs
improvement.  I have some updates pending for it.

- Eric

Linux MTD discussion mailing list

  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26 22:41 [PATCH v7 00/16] fscrypt: key management improvements Eric Biggers
2019-07-26 22:41 ` [PATCH v7 01/16] fs, fscrypt: move uapi definitions to new header <linux/fscrypt.h> Eric Biggers
2019-07-28 15:08   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 02/16] fscrypt: use FSCRYPT_ prefix for uapi constants Eric Biggers
2019-07-26 22:41 ` [PATCH v7 03/16] fscrypt: use FSCRYPT_* definitions, not FS_* Eric Biggers
2019-07-26 22:41 ` [PATCH v7 04/16] fscrypt: add ->ci_inode to fscrypt_info Eric Biggers
2019-07-28 15:09   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 05/16] fscrypt: refactor v1 policy key setup into keysetup_legacy.c Eric Biggers
2019-07-28 15:40   ` Theodore Y. Ts'o
2019-07-29 19:37     ` Eric Biggers
2019-07-26 22:41 ` [PATCH v7 06/16] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl Eric Biggers
2019-07-28 18:50   ` Theodore Y. Ts'o
2019-07-29 19:46     ` Eric Biggers
2019-07-29 20:14       ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 07/16] fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl Eric Biggers
2019-07-28 19:24   ` Theodore Y. Ts'o
2019-07-29 19:58     ` Eric Biggers
2019-07-31 18:38       ` Eric Biggers
2019-07-31 23:38         ` Theodore Y. Ts'o
2019-08-01  1:11           ` Eric Biggers [this message]
2019-08-01  5:31             ` [f2fs-dev] " Theodore Y. Ts'o
2019-08-01 18:35               ` Eric Biggers
2019-08-01 18:46                 ` Eric Biggers
2019-08-01 22:04               ` Eric Biggers
2019-08-02  4:38                 ` Eric Biggers
2019-08-12 14:16                   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 08/16] fscrypt: add FS_IOC_GET_ENCRYPTION_KEY_STATUS ioctl Eric Biggers
2019-07-28 19:30   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 09/16] fscrypt: add an HKDF-SHA512 implementation Eric Biggers
2019-07-28 19:39   ` Theodore Y. Ts'o
2019-07-29 20:29     ` Eric Biggers
2019-07-29 21:42       ` James Bottomley
2019-07-26 22:41 ` [PATCH v7 10/16] fscrypt: v2 encryption policy support Eric Biggers
2019-07-28 21:17   ` Theodore Y. Ts'o
2019-07-29 20:46     ` Eric Biggers
2019-07-26 22:41 ` [PATCH v7 11/16] fscrypt: allow unprivileged users to add/remove keys for v2 policies Eric Biggers
2019-07-28 21:22   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 12/16] fscrypt: require that key be added when setting a v2 encryption policy Eric Biggers
2019-07-28 21:24   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 13/16] ext4: wire up new fscrypt ioctls Eric Biggers
2019-07-28 21:24   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 14/16] f2fs: " Eric Biggers
2019-07-30  0:36   ` Jaegeuk Kim
2019-08-02  8:10   ` Chao Yu
2019-08-02 17:31     ` Eric Biggers
2019-08-04  9:42       ` [f2fs-dev] " Chao Yu
2019-07-26 22:41 ` [PATCH v7 15/16] ubifs: " Eric Biggers
2019-07-30  0:39   ` Theodore Y. Ts'o
2019-07-26 22:41 ` [PATCH v7 16/16] fscrypt: document the new ioctls and policy version Eric Biggers
2019-07-29  2:00   ` Theodore Y. Ts'o
2019-07-29 21:36     ` Eric Biggers

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190801011140.GB687@sol.localdomain \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on

Archives are clonable:
	git clone --mirror linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ \
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone