| Message ID | 20241124-work-cred-v1-0-f352241c3970@kernel.org |
|---|---|
| Headers | show |
| Series | cred: rework {override,revert}_creds() | expand |
On Sun, Nov 24, 2024 at 2:44 PM Christian Brauner <brauner@kernel.org> wrote: > > For the v6.13 cycle we switched overlayfs to a variant of > override_creds() that doesn't take an extra reference. To this end I > suggested introducing {override,revert}_creds_light() which overlayfs > could use. > > This seems to work rather well. As Linus correctly points out that we > should look into unifying both and simply make {override,revert}_creds() > do what {override,revert}_creds_light() currently does. Caller's that > really need the extra reference count can take it manually. > > This series does all that. Afaict, most callers can be directly > converted over and can avoid the extra reference count completely. > > Lightly tested. FWIW, your work.cred branch passes the overlayfs tests. Thanks, Amir. > > --- > Christian Brauner (26): > tree-wide: s/override_creds()/override_creds_light(get_new_cred())/g > cred: return old creds from revert_creds_light() > tree-wide: s/revert_creds()/put_cred(revert_creds_light())/g > cred: remove old {override,revert}_creds() helpers > tree-wide: s/override_creds_light()/override_creds()/g > tree-wide: s/revert_creds_light()/revert_creds()/g > firmware: avoid pointless reference count bump > sev-dev: avoid pointless cred reference count bump > target_core_configfs: avoid pointless cred reference count bump > aio: avoid pointless cred reference count bump > binfmt_misc: avoid pointless cred reference count bump > coredump: avoid pointless cred reference count bump > nfs/localio: avoid pointless cred reference count bumps > nfs/nfs4idmap: avoid pointless reference count bump > nfs/nfs4recover: avoid pointless cred reference count bump > nfsfh: avoid pointless cred reference count bump > open: avoid pointless cred reference count bump > ovl: avoid pointless cred reference count bump > cifs: avoid pointless cred reference count bump > cifs: avoid pointless cred reference count bump > smb: avoid pointless cred reference count bump > io_uring: avoid pointless cred reference count bump > acct: avoid pointless reference count bump > cgroup: avoid pointless cred reference count bump > trace: avoid pointless cred reference count bump > dns_resolver: avoid pointless cred reference count bump > > drivers/base/firmware_loader/main.c | 3 +-- > drivers/crypto/ccp/sev-dev.c | 2 +- > drivers/target/target_core_configfs.c | 3 +-- > fs/aio.c | 3 +-- > fs/backing-file.c | 20 +++++++------- > fs/cachefiles/internal.h | 4 +-- > fs/nfsd/auth.c | 4 +-- > fs/nfsd/filecache.c | 2 +- > fs/nfsd/nfs4recover.c | 3 +-- > fs/nfsd/nfsfh.c | 1 - > fs/open.c | 10 ++----- > fs/overlayfs/copy_up.c | 6 ++--- > fs/overlayfs/dir.c | 4 +-- > fs/overlayfs/util.c | 4 +-- > fs/smb/server/smb_common.c | 4 +-- > include/linux/cred.h | 14 ++++------ > kernel/cred.c | 50 ----------------------------------- > kernel/trace/trace_events_user.c | 3 +-- > 18 files changed, 35 insertions(+), 105 deletions(-) > --- > base-commit: 228a1157fb9fec47eb135b51c0202b574e079ebf > change-id: 20241124-work-cred-349b65450082 >
On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@kernel.org> wrote: > > This series does all that. Afaict, most callers can be directly > converted over and can avoid the extra reference count completely. > > Lightly tested. Thanks, this looks good to me. I only had two reactions: (a) I was surprised that using get_new_cred() apparently "just worked". I was expecting us to have cases where the cred was marked 'const', because I had this memory of us actively marking things const to make sure people didn't play games with modifying the creds in-place (and then casting away the const just for ref updates). But apparently that's never the case for override_creds() users, so your patch actually ended up even simpler than I expected in that you didn't end up needing any new helper for just incrementing the refcount on a const cred. (b) a (slight) reaction was to wish for a short "why" on the pointless reference bumps partly to show that it was thought about, but also partly to discourage people from doing it entirely mindlessly in other cases. I mean, sometimes the reference bumps were just obviously pointless because they ended up being right next to each other after being exposed, like the get/put pattern in access_override_creds(). But in some other cases, like the aio_write case, I think it would have been good to just say "The refcount is held by iocb->fsync.creds that cannot change over the operation" or similar. Or - very similarly - the binfmt_misc uses "file->f_cred", and again, file->f_cred is set at open time and never changed, so we can rely on it staying around for the file lifetime. I actually don't know if there were any exceptions to this (ie cases where the source of the override cred could actually go away from under us during the operation) where you didn't end up removing the refcount games as a result. You did have a couple of cases where you actually explained why the bump wasn't necessary, but there were a couple where I would have wished for that "the reference count is held by X, which is stable over the whole sequence" kind of notes. But not a big deal. Even in this form, I think this is a clear and good improvement. Thanks, Linus
On Sun, Nov 24, 2024 at 10:00:24AM -0800, Linus Torvalds wrote: > On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@kernel.org> wrote: > > > > This series does all that. Afaict, most callers can be directly > > converted over and can avoid the extra reference count completely. > > > > Lightly tested. > > Thanks, this looks good to me. I only had two reactions: > > (a) I was surprised that using get_new_cred() apparently "just worked". There's only one case and that's io_uring where we can just cast because we only need it temporarily during the conversion part of the patch series. Later we don't take any reference count anymore in io_uring. > (b) a (slight) reaction was to wish for a short "why" on the > pointless reference bumps Yeah, sorry for some of the patches I just quickly jotted down the same line in the commit message. I updated all those commit messages with actual explanations why that's safe. > But not a big deal. Even in this form, I think this is a clear and > good improvement. Cool.
On Sun, Nov 24, 2024 at 7:00 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@kernel.org> wrote: > > > > This series does all that. Afaict, most callers can be directly > > converted over and can avoid the extra reference count completely. > > > > Lightly tested. > > Thanks, this looks good to me. I only had two reactions: > > (a) I was surprised that using get_new_cred() apparently "just worked". > > I was expecting us to have cases where the cred was marked 'const', > because I had this memory of us actively marking things const to make > sure people didn't play games with modifying the creds in-place (and > then casting away the const just for ref updates). > > But apparently that's never the case for override_creds() users, so > your patch actually ended up even simpler than I expected in that you > didn't end up needing any new helper for just incrementing the > refcount on a const cred. > > (b) a (slight) reaction was to wish for a short "why" on the > pointless reference bumps > > partly to show that it was thought about, but also partly to > discourage people from doing it entirely mindlessly in other cases. > > I mean, sometimes the reference bumps were just obviously pointless > because they ended up being right next to each other after being > exposed, like the get/put pattern in access_override_creds(). > > But in some other cases, like the aio_write case, I think it would > have been good to just say > > "The refcount is held by iocb->fsync.creds that cannot change over > the operation" > > or similar. Or - very similarly - the binfmt_misc uses "file->f_cred", > and again, file->f_cred is set at open time and never changed, so we > can rely on it staying around for the file lifetime. > > I actually don't know if there were any exceptions to this (ie cases > where the source of the override cred could actually go away from > under us during the operation) where you didn't end up removing the > refcount games as a result. I was asking myself the same question. I see that cachefiles_{begin,end}_secure() bump the refcount, but they mostly follow a very similar pattern to the cases that do not bump the refcount, so I wonder if you left this out because they were hidden in those inline helpers or because of the non-trivial case of cachefiles_determine_cache_security() which replaces the 'master' cache_creds? Other that that, I stared at the creds code in nfsd_file_acquire_local() and nfsd_setuser() more than I would like to admit, with lines like: /* discard any old override before preparing the new set */ put_cred(revert_creds(get_cred(current_real_cred()))); And my only conclusion was this code is complicated enough, so it'd better not use borrowed creds.. Thanks, Amir.
On Sun, 2024-11-24 at 14:43 +0100, Christian Brauner wrote: > For the v6.13 cycle we switched overlayfs to a variant of > override_creds() that doesn't take an extra reference. To this end I > suggested introducing {override,revert}_creds_light() which overlayfs > could use. > > This seems to work rather well. As Linus correctly points out that we > should look into unifying both and simply make {override,revert}_creds() > do what {override,revert}_creds_light() currently does. Caller's that > really need the extra reference count can take it manually. > > This series does all that. Afaict, most callers can be directly > converted over and can avoid the extra reference count completely. > > Lightly tested. > > --- > Christian Brauner (26): > tree-wide: s/override_creds()/override_creds_light(get_new_cred())/g > cred: return old creds from revert_creds_light() > tree-wide: s/revert_creds()/put_cred(revert_creds_light())/g > cred: remove old {override,revert}_creds() helpers > tree-wide: s/override_creds_light()/override_creds()/g > tree-wide: s/revert_creds_light()/revert_creds()/g > firmware: avoid pointless reference count bump > sev-dev: avoid pointless cred reference count bump > target_core_configfs: avoid pointless cred reference count bump > aio: avoid pointless cred reference count bump > binfmt_misc: avoid pointless cred reference count bump > coredump: avoid pointless cred reference count bump > nfs/localio: avoid pointless cred reference count bumps > nfs/nfs4idmap: avoid pointless reference count bump > nfs/nfs4recover: avoid pointless cred reference count bump > nfsfh: avoid pointless cred reference count bump > open: avoid pointless cred reference count bump > ovl: avoid pointless cred reference count bump > cifs: avoid pointless cred reference count bump > cifs: avoid pointless cred reference count bump > smb: avoid pointless cred reference count bump > io_uring: avoid pointless cred reference count bump > acct: avoid pointless reference count bump > cgroup: avoid pointless cred reference count bump > trace: avoid pointless cred reference count bump > dns_resolver: avoid pointless cred reference count bump > > drivers/base/firmware_loader/main.c | 3 +-- > drivers/crypto/ccp/sev-dev.c | 2 +- > drivers/target/target_core_configfs.c | 3 +-- > fs/aio.c | 3 +-- > fs/backing-file.c | 20 +++++++------- > fs/cachefiles/internal.h | 4 +-- > fs/nfsd/auth.c | 4 +-- > fs/nfsd/filecache.c | 2 +- > fs/nfsd/nfs4recover.c | 3 +-- > fs/nfsd/nfsfh.c | 1 - > fs/open.c | 10 ++----- > fs/overlayfs/copy_up.c | 6 ++--- > fs/overlayfs/dir.c | 4 +-- > fs/overlayfs/util.c | 4 +-- > fs/smb/server/smb_common.c | 4 +-- > include/linux/cred.h | 14 ++++------ > kernel/cred.c | 50 ----------------------------------- > kernel/trace/trace_events_user.c | 3 +-- > 18 files changed, 35 insertions(+), 105 deletions(-) > --- > base-commit: 228a1157fb9fec47eb135b51c0202b574e079ebf > change-id: 20241124-work-cred-349b65450082 > > Nice work. Looks like a fairly straightforward changeover and the new API seems more intuitive. You can add: Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Nov 25, 2024 at 01:55:25PM +0100, Amir Goldstein wrote: > On Sun, Nov 24, 2024 at 7:00 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Sun, 24 Nov 2024 at 05:44, Christian Brauner <brauner@kernel.org> wrote: > > > > > > This series does all that. Afaict, most callers can be directly > > > converted over and can avoid the extra reference count completely. > > > > > > Lightly tested. > > > > Thanks, this looks good to me. I only had two reactions: > > > > (a) I was surprised that using get_new_cred() apparently "just worked". > > > > I was expecting us to have cases where the cred was marked 'const', > > because I had this memory of us actively marking things const to make > > sure people didn't play games with modifying the creds in-place (and > > then casting away the const just for ref updates). > > > > But apparently that's never the case for override_creds() users, so > > your patch actually ended up even simpler than I expected in that you > > didn't end up needing any new helper for just incrementing the > > refcount on a const cred. > > > > (b) a (slight) reaction was to wish for a short "why" on the > > pointless reference bumps > > > > partly to show that it was thought about, but also partly to > > discourage people from doing it entirely mindlessly in other cases. > > > > I mean, sometimes the reference bumps were just obviously pointless > > because they ended up being right next to each other after being > > exposed, like the get/put pattern in access_override_creds(). > > > > But in some other cases, like the aio_write case, I think it would > > have been good to just say > > > > "The refcount is held by iocb->fsync.creds that cannot change over > > the operation" > > > > or similar. Or - very similarly - the binfmt_misc uses "file->f_cred", > > and again, file->f_cred is set at open time and never changed, so we > > can rely on it staying around for the file lifetime. > > > > I actually don't know if there were any exceptions to this (ie cases > > where the source of the override cred could actually go away from > > under us during the operation) where you didn't end up removing the > > refcount games as a result. > > I was asking myself the same question. > > I see that cachefiles_{begin,end}_secure() bump the refcount, but they > mostly follow a very similar pattern to the cases that do not bump the refcount, > so I wonder if you left this out because they were hidden in those > inline helpers > or because of the non-trivial case of cachefiles_determine_cache_security() > which replaces the 'master' cache_creds? > > Other that that, I stared at the creds code in nfsd_file_acquire_local() and > nfsd_setuser() more than I would like to admit, with lines like: > > /* discard any old override before preparing the new set */ > put_cred(revert_creds(get_cred(current_real_cred()))); > > And my only conclusion was this code is complicated enough, > so it'd better not use borrowed creds.. I actually ported cachefilesd and and nfsd in v2. I simply missed them.
For the v6.13 cycle we switched overlayfs to a variant of override_creds() that doesn't take an extra reference. To this end I suggested introducing {override,revert}_creds_light() which overlayfs could use. This seems to work rather well. As Linus correctly points out that we should look into unifying both and simply make {override,revert}_creds() do what {override,revert}_creds_light() currently does. Caller's that really need the extra reference count can take it manually. This series does all that. Afaict, most callers can be directly converted over and can avoid the extra reference count completely. Lightly tested. --- Christian Brauner (26): tree-wide: s/override_creds()/override_creds_light(get_new_cred())/g cred: return old creds from revert_creds_light() tree-wide: s/revert_creds()/put_cred(revert_creds_light())/g cred: remove old {override,revert}_creds() helpers tree-wide: s/override_creds_light()/override_creds()/g tree-wide: s/revert_creds_light()/revert_creds()/g firmware: avoid pointless reference count bump sev-dev: avoid pointless cred reference count bump target_core_configfs: avoid pointless cred reference count bump aio: avoid pointless cred reference count bump binfmt_misc: avoid pointless cred reference count bump coredump: avoid pointless cred reference count bump nfs/localio: avoid pointless cred reference count bumps nfs/nfs4idmap: avoid pointless reference count bump nfs/nfs4recover: avoid pointless cred reference count bump nfsfh: avoid pointless cred reference count bump open: avoid pointless cred reference count bump ovl: avoid pointless cred reference count bump cifs: avoid pointless cred reference count bump cifs: avoid pointless cred reference count bump smb: avoid pointless cred reference count bump io_uring: avoid pointless cred reference count bump acct: avoid pointless reference count bump cgroup: avoid pointless cred reference count bump trace: avoid pointless cred reference count bump dns_resolver: avoid pointless cred reference count bump drivers/base/firmware_loader/main.c | 3 +-- drivers/crypto/ccp/sev-dev.c | 2 +- drivers/target/target_core_configfs.c | 3 +-- fs/aio.c | 3 +-- fs/backing-file.c | 20 +++++++------- fs/cachefiles/internal.h | 4 +-- fs/nfsd/auth.c | 4 +-- fs/nfsd/filecache.c | 2 +- fs/nfsd/nfs4recover.c | 3 +-- fs/nfsd/nfsfh.c | 1 - fs/open.c | 10 ++----- fs/overlayfs/copy_up.c | 6 ++--- fs/overlayfs/dir.c | 4 +-- fs/overlayfs/util.c | 4 +-- fs/smb/server/smb_common.c | 4 +-- include/linux/cred.h | 14 ++++------ kernel/cred.c | 50 ----------------------------------- kernel/trace/trace_events_user.c | 3 +-- 18 files changed, 35 insertions(+), 105 deletions(-) --- base-commit: 228a1157fb9fec47eb135b51c0202b574e079ebf change-id: 20241124-work-cred-349b65450082