[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-fsdevel
Subject:    Re: [RFC PATCH v8 09/24] ceph: add ability to set fscrypt_auth via setattr
From:       Eric Biggers <ebiggers () kernel ! org>
Date:       2021-08-31 17:54:46
Message-ID: YS5s5mYZtc3r+K/E () sol ! localdomain
[Download RAW message or body]

On Tue, Aug 31, 2021 at 09:50:32AM -0400, Jeff Layton wrote:
> > > > > +		/* It should never be re-set once set */
> > > > > +		WARN_ON_ONCE(ci->fscrypt_auth);
> > > > > +
> > > > Maybe this should return -EEXIST if already set ?
> > > > 
> > > I don't know. In general, once the context is set on an inode, we
> > > shouldn't ever reset it. That said, I think we might need to allow
> > > admins to override an existing context if it's corrupted.
> > > 
> > > So, that's the rationale for the WARN_ON_ONCE. The admins should never
> > > do this under normal circumstances but they do have the ability to
> > > change it if needed (and we'll see a warning in the logs in that case).
> > 
> > I may miss some code in the fs/crypto/ layer.
> > 
> > I readed that once the directory/file has set the policy context, it 
> > will just return 0 if the new one matches the existence, if not match it 
> > will return -EEXIST, or will try to call ceph layer to set it.
> > 
> > So once this is set, my understanding is that it shouldn't be here ?
> > 
> 
> Where did you read that? If we have documented semantics we need to
> follow here, then we should change it to comply with them.
> 

That is how FS_IOC_SET_ENCRYPTION_POLICY behaves, but the check for an existing
policy already happens in fscrypt_ioctl_set_policy(), so ->set_context doesn't
need to worry about it.

- Eric
[prev in list] [next in list] [prev in thread] [next in thread]