* [PATCH v4 3/3] ima: add new critical data record to measure log trim
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve, chenste,
gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>
Add a new critical data record to measure the trimming event when
ima event records are deleted since system boot up.
If all IMA event logs are saved in the userspace, use this log to get total
numbers of records deleted since system boot up at that point.
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
security/integrity/ima/ima_fs.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 7f805ab62f6c..1d6befa51044 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -43,6 +43,7 @@ static int valid_policy = 1;
#define IMA_LOG_TRIM_REQ_NUM_LENGTH 15
#define IMA_LOG_TRIM_REQ_TOTAL_LENGTH 32
+#define IMA_LOG_TRIM_EVENT_LEN 256
static long trimcount;
/* mutex protects atomicity of trimming measurement list
@@ -364,6 +365,22 @@ static const struct file_operations ima_ascii_measurements_ops = {
.release = ima_measurements_release,
};
+static void ima_measure_trim_event(void)
+{
+ char ima_log_trim_event[IMA_LOG_TRIM_EVENT_LEN];
+ struct timespec64 ts;
+ u64 time_ns;
+ int n;
+
+ ktime_get_real_ts64(&ts);
+ time_ns = (u64)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+ n = scnprintf(ima_log_trim_event, IMA_LOG_TRIM_EVENT_LEN,
+ "time= %llu; number= %lu;", time_ns, trimcount);
+
+ ima_measure_critical_data("ima_log_trim", "trim ima event logs",
+ ima_log_trim_event, n, false, NULL, 0);
+}
+
static int ima_log_trim_open(struct inode *inode, struct file *file)
{
bool write = !!(file->f_mode & FMODE_WRITE);
@@ -438,6 +455,8 @@ static ssize_t ima_log_trim_write(struct file *file,
goto out;
trimcount += ret;
+ if (ret > 0)
+ ima_measure_trim_event();
ret = datalen;
out:
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()
From: NeilBrown @ 2026-02-06 0:21 UTC (permalink / raw)
To: Jeff Layton
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Miklos Szeredi, Amir Goldstein, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <7fbfbeb0d57484172304b727bd888d1a1105f96d.camel@kernel.org>
On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > Instead of explicitly locking the parent and performing a look up in
> > apparmor, use simple_start_creating(), and then simple_done_creating()
> > to unlock and drop the dentry.
> >
> > This removes the need to check for an existing entry (as
> > simple_start_creating() acts like an exclusive create and can return
> > -EEXIST), simplifies error paths, and keeps dir locking code
> > centralised.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
> > 1 file changed, 9 insertions(+), 29 deletions(-)
> >
> > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> > index 907bd2667e28..7f78c36e6e50 100644
> > --- a/security/apparmor/apparmorfs.c
> > +++ b/security/apparmor/apparmorfs.c
> > @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
> >
> > dir = d_inode(parent);
> >
> > - inode_lock(dir);
> > - dentry = lookup_noperm(&QSTR(name), parent);
> > + dentry = simple_start_creating(parent, name);
> > if (IS_ERR(dentry)) {
> > error = PTR_ERR(dentry);
> > - goto fail_lock;
> > - }
> > -
> > - if (d_really_is_positive(dentry)) {
> > - error = -EEXIST;
> > - goto fail_dentry;
> > + goto fail;
> > }
> >
> > error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> > + simple_done_creating(dentry);
> > if (error)
> > - goto fail_dentry;
> > - inode_unlock(dir);
> > -
> > - return dentry;
> > -
> > -fail_dentry:
> > - dput(dentry);
> > -
> > -fail_lock:
> > - inode_unlock(dir);
> > + goto fail;
> > + return 0;
>
> As KTR points out, this should be "return NULL;"
Actually it should be "return dentry;" which is what the original code
did.
I've no idea how it became 0...
Callers of aafs_create() will silently treat NULL as failure.
>
> > +fail:
> > simple_release_fs(&aafs_mnt, &aafs_count);
> > -
> > return ERR_PTR(error);
> > }
> >
> > @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
> > if (error)
> > return error;
> >
> > - inode_lock(d_inode(parent));
> > - dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> > + dentry = simple_start_creating(parent, NULL_FILE_NAME);
> > if (IS_ERR(dentry)) {
> > error = PTR_ERR(dentry);
> > goto out;
> > @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
> > inode = new_inode(parent->d_inode->i_sb);
> > if (!inode) {
> > error = -ENOMEM;
> > - goto out1;
> > + goto out;
> > }
> >
> > inode->i_ino = get_next_ino();
> > @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
> > aa_null.dentry = dget(dentry);
> > aa_null.mnt = mntget(mount);
> >
> > - error = 0;
> > -
> > -out1:
> > - dput(dentry);
> > out:
> > - inode_unlock(d_inode(parent));
> > + simple_done_creating(dentry);
> > simple_release_fs(&mount, &count);
> > return error;
> > }
> >
> > -
> > -
> > static const char *policy_get_link(struct dentry *dentry,
> > struct inode *inode,
> > struct delayed_call *done)
>
> Assuming you fix the minor problem above.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: NeilBrown @ 2026-02-06 0:43 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jeff Layton, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, linux-kernel,
netfs, linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <CAOQ4uxh_Ugyy9=Vx_XOzWMTdhqVx6kAu43q+F+afhNF_Zv_9TA@mail.gmail.com>
On Fri, 06 Feb 2026, Amir Goldstein wrote:
> On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > >
> > > The primary purpose of this patch is to remove the locking from
> > > ovl_lookup_real_one() as part of centralising all locking of directories
> > > for name operations.
> > >
> > > The locking here isn't needed. By performing consistency tests after
> > > the lookup we can be sure that the result of the lookup was valid at
> > > least for a moment, which is all the original code promised.
> > >
> > > lookup_noperm_unlocked() is used for the lookup and it will take the
> > > lock if needed only where it is needed.
> > >
> > > Also:
> > > - don't take a reference to real->d_parent. The parent is
> > > only use for a pointer comparison, and no reference is needed for
> > > that.
> > > - Several "if" statements have a "goto" followed by "else" - the
> > > else isn't needed: the following statement can directly follow
> > > the "if" as a new statement
> > > - Use a consistent pattern of setting "err" before performing a test
> > > and possibly going to "fail".
> > > - remove the "out" label (now that we don't need to dput(parent) or
> > > unlock) and simply return from fail:.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > > fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> > > 1 file changed, 26 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index 83f80fdb1567..dcd28ffc4705 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > > struct dentry *real,
> > > const struct ovl_layer *layer)
> > > {
> > > - struct inode *dir = d_inode(connected);
> > > - struct dentry *this, *parent = NULL;
> > > + struct dentry *this;
> > > struct name_snapshot name;
> > > int err;
> > >
> > > /*
> > > - * Lookup child overlay dentry by real name. The dir mutex protects us
> > > - * from racing with overlay rename. If the overlay dentry that is above
> > > - * real has already been moved to a parent that is not under the
> > > - * connected overlay dir, we return -ECHILD and restart the lookup of
> > > - * connected real path from the top.
> > > + * @connected is a directory in the overlay and @real is an object
> > > + * on @layer which is expected to be a child of @connected.
> > > + * The goal is to return a dentry from the overlay which corresponds
>
> As the header comment already says:
> "...return a connected overlay dentry whose real dentry is @real"
>
> The wording "corresponds to @real" reduces clarity IMO.
Ok, I'll rephrase.
>
> > > + * to @real. This is done by looking up the name from @real in
> > > + * @connected and checking that the result meets expectations.
> > > + *
> > > + * Return %-ECHILD if the parent of @real no-longer corresponds to
> > > + * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > > + * correspond to @real.
> > > */
>
> I dislike kernel-doc inside code comments.
> I think this is actively discouraged and I haven't found a single example
> of this style in fs code.
>
> If you want to keep this format, please lift the comment to function
> header comment - it is anyway a very generic comment that explains the
> function in general.
OK, I'll remove the formatting or move it - not sure which.
I find that with parameter names like "connected" and "real", some sort
of syntax helps.
>
> > > - inode_lock_nested(dir, I_MUTEX_PARENT);
> > > - err = -ECHILD;
> > > - parent = dget_parent(real);
> > > - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > > - goto fail;
> > >
> > > - /*
> > > - * We also need to take a snapshot of real dentry name to protect us
> > > - * from racing with underlying layer rename. In this case, we don't
> > > - * care about returning ESTALE, only from dereferencing a free name
> > > - * pointer because we hold no lock on the real dentry.
> > > - */
> > > take_dentry_name_snapshot(&name, real);
> > > - /*
> > > - * No idmap handling here: it's an internal lookup.
> > > - */
> > > - this = lookup_noperm(&name.name, connected);
> > > + this = lookup_noperm_unlocked(&name.name, connected);
> > > release_dentry_name_snapshot(&name);
> > > +
> > > + err = -ECHILD;
> > > + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > > + goto fail;
> > > +
> > > err = PTR_ERR(this);
> > > - if (IS_ERR(this)) {
> > > + if (IS_ERR(this))
> > > goto fail;
> > > - } else if (!this || !this->d_inode) {
> > > - dput(this);
> > > - err = -ENOENT;
> > > +
> > > + err = -ENOENT;
> > > + if (!this || !this->d_inode)
> > > goto fail;
> > > - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > > - dput(this);
> > > - err = -ESTALE;
> > > +
> > > + err = -ESTALE;
> > > + if (ovl_dentry_real_at(this, layer->idx) != real)
> > > goto fail;
> > > - }
> > >
> > > -out:
> > > - dput(parent);
> > > - inode_unlock(dir);
> > > return this;
> > >
> > > fail:
> > > pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> > > real, layer->idx, connected, err);
> > > - this = ERR_PTR(err);
> > > - goto out;
> > > + if (!IS_ERR(this))
> > > + dput(this);
> > > + return ERR_PTR(err);
> > > }
> > >
> > > static struct dentry *ovl_lookup_real(struct super_block *sb,
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> Otherwise, it looks fine.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: NeilBrown @ 2026-02-06 1:11 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <CAOQ4uxh-MLgwZCstwr6HyPXHVRmtj2F_=xS8pE3FN6Ex-wex4w@mail.gmail.com>
On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > When ovl_create_real() is used to create a file on the upper filesystem
> > it needs to return the resulting dentry - positive and hashed.
> > It is usually the case the that dentry passed to the create function
> > (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> > filesystem may unhash that dentry forcing a repeat lookup next time the
> > name is wanted.
> >
> > So ovl_create_real() must be (and is) aware of this and prepared to
> > perform that lookup to get a hash positive dentry.
> >
> > This is currently done under that same directory lock that provided
> > exclusion for the create. Proposed changes to locking will make this
> > not possible - as the name, rather than the directory, will be locked.
> > The new APIs provided for lookup and locking do not and cannot support
> > this pattern.
> >
> > The lock isn't needed. ovl_create_real() can drop the lock and then get
> > a new lock for the lookup - then check that the lookup returned the
> > correct inode. In a well-behaved configuration where the upper
> > filesystem is not being modified by a third party, this will always work
> > reliably, and if there are separate modification it will fail cleanly.
> >
> > So change ovl_create_real() to drop the lock and call
> > ovl_start_creating_upper() to find the correct dentry. Note that
> > start_creating doesn't fail if the name already exists.
> >
> > This removes the only remaining use of ovl_lookup_upper, so it is
> > removed.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/overlayfs/dir.c | 24 ++++++++++++++++++------
> > fs/overlayfs/overlayfs.h | 7 -------
> > 2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index ff3dbd1ca61f..ec08904d084d 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > err = -EIO;
> > } else if (d_unhashed(newdentry)) {
> > struct dentry *d;
> > + struct name_snapshot name;
> > /*
> > * Some filesystems (i.e. casefolded) may return an unhashed
> > - * negative dentry from the ovl_lookup_upper() call before
> > + * negative dentry from the ovl_start_creating_upper() call before
> > * ovl_create_real().
>
>
> According to the new locking rules, if the hashed dentry itself is
> the synchronization object, is it going to be allowed to
> filesystem to unhash the dentry while the dentry still in the
> "creating" scope? It is hard for me to wrap my head around this.
It can be confusing....
It will be important for the name the remain locked (and hashed) until
the operation (create, remove, rename) either succeeds or fails. So
leaving a dentry unhashed will be OK providing a subsequent lookup will
also succeed or fail in the same way. The caller must be able to use
the dentry to access the object (i.e. the inode) on success, but they
is nothing in POSIX that requires that the object still has any
particular name.
>
> Or do we need this here because some filesystems (casefold in
> particular) are not going to support parallel creations?
There is no reason that a casefolding filesystem would not support parallel
ops. And it isn't just casefolding that acts like this. At least one of
the special filesystems (tracefs maybe) always unhashes on create. You
only ever get a hashed positive dentry as a result of lookup.
(overlayfs would never see this case of course).
>
> > * In that case, lookup again after making the newdentry
> > * positive, so ovl_create_upper() always returns a hashed
> > * positive dentry.
> > + * As we have to drop the lock before the lookup a race
> > + * could result in a lookup failure. In that case we return
> > + * an error.
> > */
> > - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > - newdentry->d_name.len);
> > - dput(newdentry);
> > - if (IS_ERR_OR_NULL(d))
> > + take_dentry_name_snapshot(&name, newdentry);
> > + end_creating_keep(newdentry);
> > + d = ovl_start_creating_upper(ofs, parent, &name.name);
> > + release_dentry_name_snapshot(&name);
>
> OK. not saying no to this (yet) but I have to admit that it is pretty
> ugly that the callers of ovl_create_real() want to create a specific
> stable name, which is could be passed in as const char *name
> and yet we end up doing this weird dance here just to keep the name
> from newdentry.
There are three callers of ovl_create_real()
ovl_lookup_or_create() does have a "const char *name".
ovl_create_upper() has a stable dentry from which it can copy a QSTR
ovl_create_temp() would need some sort of dance to keep hold of the
temporary name that was allocated.
If it weren't for ovl_create_temp() I would agree with you.
Though we could have the three callers of ovl_start_creating_temp() pass a
"char name[OVL_TEMPNAME_SIZE]" in, then ovl_create_temp() would have
easy access.
I could do that if you like.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: NeilBrown @ 2026-02-06 1:18 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <CAOQ4uxi3bNYq1b4=qL-JLi19hRwurntfLZXhUMVL003NarBdGg@mail.gmail.com>
On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > This function is unused.
> >
>
> I am confused.
> What was this "fix" fixing an unused function:
>
> e9c70084a64e5 ovl: fail ovl_lock_rename_workdir() if either target is unhashed
>
> What am I missing?
>
Commit 833d2b3a072f ("Add start_renaming_two_dentries()")
removed the last use of ovl_lock_rename_workdir() earlier, but in a
different branch.
e9c was committed upstream Nov 28th v6.18~7
833 was committed upstream Dec 1st v6.19-rc1~240
> Otherwise, feel free to add:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 03/13] libfs: change simple_done_creating() to use end_creating()
From: NeilBrown @ 2026-02-06 2:13 UTC (permalink / raw)
To: Jeff Layton
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Miklos Szeredi, Amir Goldstein, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <8d907c67ccab1db0e7bcabe0c34c66722a2970e2.camel@kernel.org>
On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > simple_done_creating() and end_creating() are identical.
> > So change the former to use the latter. This further centralises
> > unlocking of directories.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/libfs.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index f1860dff86f2..db18b53fc189 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
> > /* parent must have been held exclusive since simple_start_creating() */
> > void simple_done_creating(struct dentry *child)
> > {
> > - inode_unlock(child->d_parent->d_inode);
> > - dput(child);
> > + end_creating(child);
> > }
> > EXPORT_SYMBOL(simple_done_creating);
>
> nit: seems like it would be better to turn this into a static inline
True ... but then it could have been a static inline anyway.
I'd rather not change it without good reason, or knowing what it was
written that way.
Al: do you have an opinion on this?
Thanks,
NeilBrown
^ permalink raw reply
* 黒字の幼児教室 投資オーナー募集
From: 幼児教室／ BBS インターナショナル @ 2026-02-06 2:28 UTC (permalink / raw)
To: linux-security-module
代表者様へ、突然のご連絡失礼いたします。
黒字運営の幼児教室オーナーとなり、
安定した収益と社会貢献を両立できる、投資オーナーを募集しています。
（運営やマネジメントの負担は一切ありません）
◇ 完全業務委託型｜黒字化教室のオーナーになれる ◇
全国200教室／業界最大規模の幼児教室ベビーパーク
◇ 詳細はこちら
https://bbs-i.net/2500a/
投資をする時、どんなポートフォリオを組んでいますか？
短期的には、利回りや回収時期が重要なのは当然ですが
長期的には、その投資が社会に与える影響を考えることが
より大きなリターンにつながる。と考える方も少なくありません。
特に、日本の未来の人材を育てる投資に
関心を持たれる方も増えています。
幼児教育は、日本の未来を支える事業と言えます。
事実、近年の大脳生理学の進歩により、
「人間の脳細胞は3歳で80%、6歳で90%完成する」という事実が明らかになり、
人生の基盤を決めるのは幼児期の教育であることが注目を集めています。
しかし、日本では幼児期の学びに対する意識が
十分に浸透していないという課題があります。
・30年間ゼロ成長という長期低迷
・不安定さを増す国際情勢
・社会保障制度への懸念
このような状況の中で、未来の日本を支えるのは、"優秀な人材"を生み出す教育です。
特に幼児教育の充実は、社会全体の成長につながる最も効率的な投資といえます。
このような未来を創るために、黒字運営の幼児教室オーナーになり、
収益を得ながら社会貢献できる仕組みを提供しています。
◇ 詳細はこちら
https://bbs-i.net/2500a/
◇ オーナー募集モデルの特徴 ◇
・完全業務委託型で、物件・人材・時間リソースなど一切不要
・すでに黒字化の教室への投資のため、投資直後から毎月のキャッシュフローを生み出す
・「売却オプション」付きで、明確な出口戦略を確保
このモデルにより、安定した収益と社会貢献の両立が実現できます。
また、分散投資の選択肢としても優れたリスクヘッジになります。
全国200教室／業界最大規模の幼児教室「ベビーパーク」のオーナーとして、
資産を増やしながら、社会貢献事業を始めませんか？
この機会に、ぜひ詳細資料をご覧ください。
最後までお読みいただき、誠にありがとうございました。
■□■───────────────────────────■□■
BBSインターナショナル株式会社
住所：東京都中央区日本橋小伝馬町2-5 メトロシティ小伝馬町7階
メールご不要の方は、下記よりお手続きをお願いいたします。
https://bbs-i.net/mail/
■□■───────────────────────────■□■
^ permalink raw reply
* [PATCH v2 v2] evm: check return values of crypto_shash functions
From: Daniel Hodges @ 2026-02-06 2:42 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, paul,
jmorris, serge, linux-integrity, linux-security-module,
linux-kernel, Daniel Hodges
In-Reply-To: <aYNprpzxppKE0Gf2@fb.com>
The crypto_shash_update() and crypto_shash_final() functions can fail
and return error codes, but their return values were not being checked
in several places in security/integrity/evm/evm_crypto.c:
- hmac_add_misc() ignored returns from crypto_shash_update() and
crypto_shash_final()
- evm_calc_hmac_or_hash() ignored returns from crypto_shash_update()
- evm_init_hmac() ignored returns from crypto_shash_update()
If these hash operations fail silently, the resulting HMAC could be
invalid or incomplete, which could weaken the integrity verification
security that EVM provides.
This patch converts hmac_add_misc() from void to int return type and
adds proper error checking and propagation for all crypto_shash_*
function calls. All callers are updated to handle the new return values.
Additionally, error messages are logged when cryptographic operations
fail to provide visibility into the failure rather than silently
returning error codes.
Fixes: 66dbc325afce ("evm: re-release")
Signed-off-by: Daniel Hodges <git@danielhodges.dev>
---
security/integrity/evm/evm_crypto.c | 55 ++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a5e730ffda57..402eb1ca64ce 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -139,7 +139,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
* (Additional directory/file metadata needs to be added for more complete
* protection.)
*/
-static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
+static int hmac_add_misc(struct shash_desc *desc, struct inode *inode,
char type, char *digest)
{
struct h_misc {
@@ -149,6 +149,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
gid_t gid;
umode_t mode;
} hmac_misc;
+ int error;
memset(&hmac_misc, 0, sizeof(hmac_misc));
/* Don't include the inode or generation number in portable
@@ -169,14 +170,28 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
hmac_misc.mode = inode->i_mode;
- crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ error = crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ return error;
+ }
if ((evm_hmac_attrs & EVM_ATTR_FSUUID) &&
- type != EVM_XATTR_PORTABLE_DIGSIG)
- crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
- crypto_shash_final(desc, digest);
+ type != EVM_XATTR_PORTABLE_DIGSIG) {
+ error = crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ return error;
+ }
+ }
+ error = crypto_shash_final(desc, digest);
+ if (error) {
+ pr_err("crypto_shash_final() failed: %d\n", error);
+ return error;
+ }
pr_debug("hmac_misc: (%zu) [%*phN]\n", sizeof(struct h_misc),
(int)sizeof(struct h_misc), &hmac_misc);
+ return 0;
}
/*
@@ -260,9 +275,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
if ((req_xattr_name && req_xattr_value)
&& !strcmp(xattr->name, req_xattr_name)) {
- error = 0;
- crypto_shash_update(desc, (const u8 *)req_xattr_value,
+ error = crypto_shash_update(desc, (const u8 *)req_xattr_value,
req_xattr_value_len);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
if (is_ima)
ima_present = true;
@@ -286,15 +304,20 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
pr_debug("file %s: xattr %s size mismatch (kernel: %d, user: %d)\n",
dentry->d_name.name, xattr->name, size,
user_space_size);
- error = 0;
xattr_size = size;
- crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
+ error = crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
if (is_ima)
ima_present = true;
dump_security_xattr(xattr->name, xattr_value, xattr_size);
}
- hmac_add_misc(desc, inode, type, data->digest);
+ error = hmac_add_misc(desc, inode, type, data->digest);
+ if (error)
+ goto out;
if (inode != d_backing_inode(dentry) && iint) {
if (IS_I_VERSION(inode))
@@ -401,6 +424,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
{
struct shash_desc *desc;
const struct xattr *xattr;
+ int error;
desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
if (IS_ERR(desc)) {
@@ -412,12 +436,17 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
if (!evm_protected_xattr(xattr->name))
continue;
- crypto_shash_update(desc, xattr->value, xattr->value_len);
+ error = crypto_shash_update(desc, xattr->value, xattr->value_len);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
}
- hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+ error = hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+out:
kfree(desc);
- return 0;
+ return error;
}
/*
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v2 0/3] landlock: Refactor layer masks
From: Günther Noack @ 2026-02-06 7:32 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260128.jiethoh2Zeem@digikod.net>
Hello!
On Wed, Jan 28, 2026 at 10:31:07PM +0100, Mickaël Salaün wrote:
> On Sun, Jan 25, 2026 at 08:58:50PM +0100, Günther Noack wrote:
> > P.S.: I am open to suggestions on what the "layer masks" variables
> > should be called, because the name "layer masks" might be less
> > appropriate after this change. I have not fixed up the name
> > everywhere because fixing up the code took priority for now.
>
> Could you please clarify your thoughts and explain why this name might
> not be appropriate anymore? Any list of name proposals?
>
> If we rename the variables, this should be done in the same refactoring
> patch.
When this was an array of layer_mask_t, the name layer_masks was a
description of that underlying data type. Now that we have removed
the layer_mask_t datatype, it is not as obviously true any more.
When trying to name these variables after the "role" that they have in
their declaration context, I think of them as "unfulfilled per-layer
access requests", but that strikes me as a bit long.
For the upcoming patch set, I'm leaning towards naming these variables
just "masks", to keep it short.
> > Changes since previous versions:
> >
> > V2: (This patch set)
> >
> > * Remove the refactoring around the deny_mask_t type,
> > it is better to send that as a separate patch (mic review)
>
> Feel free to include the new dedicated patch in this series.
I'm afraid that this one did not get any further than what it already
was, and I'll have to leave it out for now. But I have it on my TODO
list.
–Günther
^ permalink raw reply
* Re: [PATCH v2 0/3] landlock: Refactor layer masks
From: Günther Noack @ 2026-02-06 7:49 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260206.9509420815f5@gnoack.org>
On Fri, Feb 06, 2026 at 08:32:06AM +0100, Günther Noack wrote:
> On Wed, Jan 28, 2026 at 10:31:07PM +0100, Mickaël Salaün wrote:
> > On Sun, Jan 25, 2026 at 08:58:50PM +0100, Günther Noack wrote:
> > > P.S.: I am open to suggestions on what the "layer masks" variables
> > > should be called, because the name "layer masks" might be less
> > > appropriate after this change. I have not fixed up the name
> > > everywhere because fixing up the code took priority for now.
> >
> > Could you please clarify your thoughts and explain why this name might
> > not be appropriate anymore? Any list of name proposals?
> >
> > If we rename the variables, this should be done in the same refactoring
> > patch.
>
> When this was an array of layer_mask_t, the name layer_masks was a
> description of that underlying data type. Now that we have removed
> the layer_mask_t datatype, it is not as obviously true any more.
>
> When trying to name these variables after the "role" that they have in
> their declaration context, I think of them as "unfulfilled per-layer
> access requests", but that strikes me as a bit long.
>
> For the upcoming patch set, I'm leaning towards naming these variables
> just "masks", to keep it short.
OK, staring at the code a bit longer, I realize that since the type is
now named "struct layer_access_masks", "layer_masks" is actually still
a reasonable shorthand. I have abbreviated that to "masks" in some
places where it is anyway clear from the context that those are the
layer access masks, but left it as "layer_masks" in places where we
also use other access masks, for disambiguation.
–Günther
^ permalink raw reply
* Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
From: Günther Noack @ 2026-02-06 8:02 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260129.xahm6Ue7raL3@digikod.net>
On Thu, Jan 29, 2026 at 05:54:01PM +0100, Mickaël Salaün wrote:
> On Thu, Jan 29, 2026 at 08:56:37AM +0100, Günther Noack wrote:
> > On Wed, Jan 28, 2026 at 10:34:02PM +0100, Mickaël Salaün wrote:
> > > > + for (int i = ARRAY_SIZE(masks->access) - 1; i >= 0; i--) {
> > >
> > > size_t i
> >
> > This is one of the two places where this didn't work.
> >
> > The loop goes from top to bottom here, and the "i >= 0" check would
> > always be true for a size_t.
> >
> > If there is a more idiomatic way to write that loop, I can switch to
> > it, but would otherwise lean towards keeping it as it is?
>
> Indeed. We can use ssize_t as in get_hierarchy().
Good point, done.
> > > > -static bool
> > > > -scope_to_request(const access_mask_t access_request,
> > > > - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> > > > +static bool scope_to_request(const access_mask_t access_request,
> > > > + struct layer_access_masks *masks)
> > > > {
> > > > - const unsigned long access_req = access_request;
> > > > - unsigned long access_bit;
> > > > + bool saw_unfulfilled_access = false;
> > > >
> > > > - if (WARN_ON_ONCE(!layer_masks))
> > > > + if (WARN_ON_ONCE(!masks))
> > > > return true;
> > > >
> > > > - for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks))
> > > > - (*layer_masks)[access_bit] = 0;
> > > > -
> > > > - return is_layer_masks_allowed(layer_masks);
> > > > + for (size_t i = 0; i < ARRAY_SIZE(masks->access); i++) {
> > > > + masks->access[i] &= access_request;
> > > > + if (masks->access[i])
> > >
> > > {
> > >
> > > > + saw_unfulfilled_access = true;
> > >
> > > break;
> > > }
> >
> > Two lines above, this loop mutates masks->access[...]:
> >
> > masks->access[i] &= access_request
> >
> > If we break the loop early, we would not actually scope it down to the
> > request entirely? Is this safe?
>
> You're right, don't add this break. BTW, would a test catch it?
Yes, the existing tests already catch that; this happens when we break early:
[08:53:12] ================= landlock_fs (7 subtests) =================
[08:53:12] [PASSED] test_no_more_access
[08:53:12] [PASSED] test_scope_to_request_with_exec_none
[08:53:12] # test_scope_to_request_with_exec_some: EXPECTATION FAILED at security/landlock/fs.c:616
[08:53:12] Expected 0 == masks.access[1], but
[08:53:12] masks.access[1] == 2 (0x2)
[08:53:12] [FAILED] test_scope_to_request_with_exec_some
[08:53:12] [PASSED] test_scope_to_request_without_access
[08:53:12] [PASSED] test_is_eacces_with_none
[08:53:12] [PASSED] test_is_eacces_with_refer
[08:53:12] [PASSED] test_is_eacces_with_write
[08:53:12] # module: landlock
[08:53:12] # landlock_fs: pass:6 fail:1 skip:0 total:7
[08:53:12] # Totals: pass:6 fail:1 skip:0 total:7
Good coverage!
–Günther
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: David Howells @ 2026-02-06 8:18 UTC (permalink / raw)
To: =?utf-8?q?Thomas_Wei=C3=9Fschuh?=
Cc: dhowells, Nathan Chancellor, Arnd Bergmann, Luis Chamberlain,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Nicolas Schier,
Daniel Gomez, Aaron Tomlin, Christophe Leroy (CS GROUP),
Nicolas Schier, Nicolas Bouchinet, Xiu Jianfeng,
Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-5-0b932db9b56b@weissschuh.net>
Thomas Weißschuh <linux@weissschuh.net> wrote:
> As both 'size_t' and 'unsigned int' are always the same size, this
> should be risk-free.
Did you mean 'unsigned long'?
David
^ permalink raw reply
* Re: [PATCH v4 02/17] powerpc/ima: Drop unnecessary check for CONFIG_MODULE_SIG
From: Nicolas Schier @ 2026-02-06 8:25 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-2-0b932db9b56b@weissschuh.net>
On Tue, Jan 13, 2026 at 01:28:46PM +0100, Thomas Weißschuh wrote:
> When CONFIG_MODULE_SIG is disabled set_module_sig_enforced() is defined
> as an empty stub, so the check is unnecessary.
> The specific configuration option for set_module_sig_enforced() is
> about to change and removing the check avoids some later churn.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> arch/powerpc/kernel/ima_arch.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v4 03/17] ima: efi: Drop unnecessary check for CONFIG_MODULE_SIG/CONFIG_KEXEC_SIG
From: Nicolas Schier @ 2026-02-06 8:25 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-3-0b932db9b56b@weissschuh.net>
On Tue, Jan 13, 2026 at 01:28:47PM +0100, Thomas Weißschuh wrote:
> When configuration settings are disabled the guarded functions are
> defined as empty stubs, so the check is unnecessary.
> The specific configuration option for set_module_sig_enforced() is
> about to change and removing the checks avoids some later churn.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> security/integrity/ima/ima_efi.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v4 04/17] module: Make mod_verify_sig() static
From: Nicolas Schier @ 2026-02-06 8:25 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-4-0b932db9b56b@weissschuh.net>
On Tue, Jan 13, 2026 at 01:28:48PM +0100, Thomas Weißschuh wrote:
> It is not used outside of signing.c.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> kernel/module/internal.h | 1 -
> kernel/module/signing.c | 2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Nicolas Schier @ 2026-02-06 8:30 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-5-0b932db9b56b@weissschuh.net>
On Tue, Jan 13, 2026 at 01:28:49PM +0100, Thomas Weißschuh wrote:
> Switching the types will make some later changes cleaner.
> size_t is also the semantically correct type for this field.
>
> As both 'size_t' and 'unsigned int' are always the same size, this
> should be risk-free.
include/uapi/asm-generic/posix_types.h states:
| * Most 32 bit architectures use "unsigned int" size_t,
| * and all 64 bit architectures use "unsigned long" size_t.
Is that statement wrong? Or did I mix up the context?
Kind regards,
Nicolas
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Thomas Weißschuh @ 2026-02-06 8:34 UTC (permalink / raw)
To: David Howells
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Nicolas Schier,
Daniel Gomez, Aaron Tomlin, Christophe Leroy (CS GROUP),
Nicolas Schier, Nicolas Bouchinet, Xiu Jianfeng,
Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <2919071.1770365933@warthog.procyon.org.uk>
On 2026-02-06 08:18:53+0000, David Howells wrote:
> Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> > As both 'size_t' and 'unsigned int' are always the same size, this
> > should be risk-free.
>
> Did you mean 'unsigned long'?
Indeed, I'll fix it for the next revision.
Thomas
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Thomas Weißschuh @ 2026-02-06 8:38 UTC (permalink / raw)
To: Nicolas Schier, Nathan Chancellor, Arnd Bergmann,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Paul Moore, James Morris, Serge E. Hallyn, Jonathan Corbet,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Naveen N Rao, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Daniel Gomez, Aaron Tomlin,
Christophe Leroy (CS GROUP), Nicolas Bouchinet, Xiu Jianfeng,
Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <aYWmkEzjvo9RrzI9@levanger>
On 2026-02-06 09:30:08+0100, Nicolas Schier wrote:
> On Tue, Jan 13, 2026 at 01:28:49PM +0100, Thomas Weißschuh wrote:
> > Switching the types will make some later changes cleaner.
> > size_t is also the semantically correct type for this field.
> >
> > As both 'size_t' and 'unsigned int' are always the same size, this
> > should be risk-free.
>
> include/uapi/asm-generic/posix_types.h states:
> | * Most 32 bit architectures use "unsigned int" size_t,
> | * and all 64 bit architectures use "unsigned long" size_t.
>
> Is that statement wrong? Or did I mix up the context?
That statement is correct. But as both 'unsigned int' and 'unsigned
long' are 32-bit wide on 32-bit Linux platforms they are compatible.
Thomas
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Nicolas Schier @ 2026-02-06 8:55 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <8fd4914d-5cff-4030-822c-98c8e76d0e60@t-8ch.de>
On Fri, Feb 06, 2026 at 09:38:07AM +0100, Thomas Weißschuh wrote:
> On 2026-02-06 09:30:08+0100, Nicolas Schier wrote:
> > On Tue, Jan 13, 2026 at 01:28:49PM +0100, Thomas Weißschuh wrote:
> > > Switching the types will make some later changes cleaner.
> > > size_t is also the semantically correct type for this field.
> > >
> > > As both 'size_t' and 'unsigned int' are always the same size, this
> > > should be risk-free.
> >
> > include/uapi/asm-generic/posix_types.h states:
> > | * Most 32 bit architectures use "unsigned int" size_t,
> > | * and all 64 bit architectures use "unsigned long" size_t.
> >
> > Is that statement wrong? Or did I mix up the context?
>
> That statement is correct. But as both 'unsigned int' and 'unsigned
> long' are 32-bit wide on 32-bit Linux platforms they are compatible.
>
>
> Thomas
sure, thanks!
Acked-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Christophe Leroy (CS GROUP) @ 2026-02-06 9:09 UTC (permalink / raw)
To: Thomas Weißschuh, Nathan Chancellor, Arnd Bergmann,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Paul Moore, James Morris, Serge E. Hallyn, Jonathan Corbet,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Naveen N Rao, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Nicolas Schier, Daniel Gomez, Aaron Tomlin,
Nicolas Schier, Nicolas Bouchinet, Xiu Jianfeng
Cc: Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <20260113-module-hashes-v4-5-0b932db9b56b@weissschuh.net>
Le 13/01/2026 à 13:28, Thomas Weißschuh a écrit :
> Switching the types will make some later changes cleaner.
> size_t is also the semantically correct type for this field.
>
> As both 'size_t' and 'unsigned int' are always the same size, this
> should be risk-free.
Are you sure ?
Some architectures have size_t as 'unsigned int', some have 'unsigned
long', some have 'unsigned long long'
https://elixir.bootlin.com/linux/v6.19-rc5/source/arch/s390/include/uapi/asm/posix_types.h#L16
https://elixir.bootlin.com/linux/v6.19-rc5/source/arch/sparc/include/uapi/asm/posix_types.h#L35
https://elixir.bootlin.com/linux/v6.19-rc5/source/arch/xtensa/include/uapi/asm/posix_types.h#L26
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/uapi/asm-generic/posix_types.h#L68
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/uapi/asm-generic/posix_types.h#L72
https://elixir.bootlin.com/linux/v6.19-rc5/source/arch/sparc/include/uapi/asm/posix_types.h#L23
https://elixir.bootlin.com/linux/v6.19-rc5/source/arch/x86/include/uapi/asm/posix_types_x32.h#L15
https://elixir.bootlin.com/linux/v6.19-rc5/source/include/uapi/asm-generic/posix_types.h#L16
Christophe
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: Thomas Weißschuh @ 2026-02-06 9:18 UTC (permalink / raw)
To: Christophe Leroy (CS GROUP)
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Nicolas Schier,
Daniel Gomez, Aaron Tomlin, Nicolas Schier, Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <ffdafd21-fe7a-44a2-86ec-0e0c2ad4238c@kernel.org>
On 2026-02-06 10:09:12+0100, Christophe Leroy (CS GROUP) wrote:
>
>
> Le 13/01/2026 à 13:28, Thomas Weißschuh a écrit :
> > Switching the types will make some later changes cleaner.
> > size_t is also the semantically correct type for this field.
> >
> > As both 'size_t' and 'unsigned int' are always the same size, this
> > should be risk-free.
> Are you sure ?
As mentioned before by David [0], this should have been 'unsigned long'
instead of 'unsigned int'. Which is also what the diff shows.
> Some architectures have size_t as 'unsigned int', some have 'unsigned long',
> some have 'unsigned long long'
(...)
[0] https://lore.kernel.org/lkml/2919071.1770365933@warthog.procyon.org.uk/
^ permalink raw reply
* Re: [PATCH v2 0/3] landlock: Refactor layer masks
From: Mickaël Salaün @ 2026-02-06 11:24 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260206.a8bf33606ef0@gnoack.org>
On Fri, Feb 06, 2026 at 08:49:39AM +0100, Günther Noack wrote:
> On Fri, Feb 06, 2026 at 08:32:06AM +0100, Günther Noack wrote:
> > On Wed, Jan 28, 2026 at 10:31:07PM +0100, Mickaël Salaün wrote:
> > > On Sun, Jan 25, 2026 at 08:58:50PM +0100, Günther Noack wrote:
> > > > P.S.: I am open to suggestions on what the "layer masks" variables
> > > > should be called, because the name "layer masks" might be less
> > > > appropriate after this change. I have not fixed up the name
> > > > everywhere because fixing up the code took priority for now.
> > >
> > > Could you please clarify your thoughts and explain why this name might
> > > not be appropriate anymore? Any list of name proposals?
> > >
> > > If we rename the variables, this should be done in the same refactoring
> > > patch.
> >
> > When this was an array of layer_mask_t, the name layer_masks was a
> > description of that underlying data type. Now that we have removed
> > the layer_mask_t datatype, it is not as obviously true any more.
> >
> > When trying to name these variables after the "role" that they have in
> > their declaration context, I think of them as "unfulfilled per-layer
> > access requests", but that strikes me as a bit long.
> >
> > For the upcoming patch set, I'm leaning towards naming these variables
> > just "masks", to keep it short.
>
> OK, staring at the code a bit longer, I realize that since the type is
> now named "struct layer_access_masks", "layer_masks" is actually still
> a reasonable shorthand. I have abbreviated that to "masks" in some
> places where it is anyway clear from the context that those are the
> layer access masks, but left it as "layer_masks" in places where we
> also use other access masks, for disambiguation.
Looks good
^ permalink raw reply
* Re: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
From: Günther Noack @ 2026-02-06 12:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260128.eiJou3fiezai@digikod.net>
Hello!
On Wed, Jan 28, 2026 at 10:31:23PM +0100, Mickaël Salaün wrote:
> On Sun, Jan 25, 2026 at 08:58:51PM +0100, Günther Noack wrote:
> > --- /dev/null
> > +++ b/tools/testing/selftests/landlock/fs_bench.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Landlock filesystem benchmark
>
> You might want to add some copyright.
Done.
> > +void usage(const char *argv0)
>
> const
Done.
> > +int build_directory(size_t depth, bool use_landlock)
>
> const
Done.
> > + if (use_landlock) {
> > + abi = syscall(SYS_landlock_create_ruleset, NULL, 0,
> > + LANDLOCK_CREATE_RULESET_VERSION);
>
> Please include wrappers.h and use the related syscall helpers. One of
> the benefit is to use __NR_* constants defined by the installed kernel
> headers.
Done.
> > + if (abi < 7)
> > + err(1, "Landlock ABI too low: got %d, wanted 7+", abi);
> > + }
> > +
> > + ruleset_fd = -1;
> > + if (use_landlock) {
> > + struct landlock_ruleset_attr attr = {
> > + .handled_access_fs =
> > + 0xffff, /* All FS access rights as of 2026-01 */
> > + };
> > + ruleset_fd = syscall(SYS_landlock_create_ruleset, &attr,
> > + sizeof(attr), 0U);
> > + if (ruleset_fd < 0)
> > + err(1, "landlock_create_ruleset");
> > + }
> > +
> > + current = open(".", O_PATH);
> > + if (current < 0)
> > + err(1, "open(.)");
> > +
> > + while (depth--) {
> > + if (use_landlock) {
> > + struct landlock_path_beneath_attr attr = {
> > + .allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> > + .parent_fd = current,
> > + };
> > + if (syscall(SYS_landlock_add_rule, ruleset_fd,
> > + LANDLOCK_RULE_PATH_BENEATH, &attr, 0) < 0)
> > + err(1, "landlock_add_rule");
> > + }
> > +
> > + if (mkdirat(current, path, 0700) < 0)
> > + err(1, "mkdirat(%s)", path);
>
> We should have a loop to build the directories, then start the timer and
> have another loop to add Landlock rules.
I have to politely push back on this; the granularity of time
measurement is not high enough and the measurement below only works
because we repeat it 100000 times. This is not the case when we
construct a Landlock ruleset, and it would IMHO be weird to build the
ruleset multiple times as well. It feels like this would better be
measured in a separate benchmark.
Adding a rule is an operation whose runtime does not depend on the
depth of the nested directories, so such a separate benchmark would
then also be simpler and wouldn't need to construct such a deeply
nested hierarchy.
> > + printf("*** Benchmark ***\n");
>
> We should probably use ksft_*() helpers in main (see
> seccomp_benchmark.c).
Among the benchmarks, the seccomp benchmark is the one exception in
that it uses these ksft_*() helpers, and it's not clear to me that it
has any benefit. These helpers are for producing TAP-formatted
output, and assume that there will be individual test cases with
success/failure results, which is not the case here. The seccomp test
uses approximate assertions about the expected timing of operations
(+-10%), but I don't think we can easily do that in our case.
I would therefore prefer to use a normal textual output format,
similar to the other benchmarks in tools/testing/kselftests.
> > + printf("%zu dirs, %zu iterations, %s landlock\n", num_subdirs,
> > + num_iterations, use_landlock ? "with" : "without");
> > +
> > + if (times(&start_time) == -1)
> > + err(1, "times");
> > +
> > + current = build_directory(num_subdirs, use_landlock);
> > +
> > + for (int i = 0; i < num_iterations; i++) {
> > + fd = openat(current, ".", O_DIRECTORY);
>
> We can use AT_EMPTY_PATH (with an empty path) instead of "."
> I guess the benchmark should not change, but better to check again.
This had to change anyway; now that I added cleanup of the created
directories, I had to use another operation here that would trigger
the path walk (file open for creation). Opening directories and
removing directories both need to continue working so that we can
later remove the directories. (See discussion below.)
> > + if (fd != -1) {
> > + if (use_landlock)
> > + errx(1, "openat succeeded, expected error");
> > +
> > + close(fd);
> > + }
> > + }
> > +
> > + if (times(&end_time) == -1)
> > + err(1, "times");
>
> The created directories should be removed here (setup and teardown).
Done.
Minor implementation remark: This is also done with explicit loops
that use openat() to walk the directory tree with file descriptors and
then unlinkat(fd, "d", ...). At this nesting depth, the paths don't
fit into PATH_MAX any more and relative dirfds are the only way to do
that AFAIK. (The directory walk function nftw(3) also breaks down
FWIW, because it uses long paths relative to cwd.)
–Günther
^ permalink raw reply
* Re: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
From: Mickaël Salaün @ 2026-02-06 12:59 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260206.e69a9f79acac@gnoack.org>
On Fri, Feb 06, 2026 at 01:24:02PM +0100, Günther Noack wrote:
> Hello!
>
> On Wed, Jan 28, 2026 at 10:31:23PM +0100, Mickaël Salaün wrote:
> > On Sun, Jan 25, 2026 at 08:58:51PM +0100, Günther Noack wrote:
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/landlock/fs_bench.c
> > > + if (abi < 7)
> > > + err(1, "Landlock ABI too low: got %d, wanted 7+", abi);
> > > + }
> > > +
> > > + ruleset_fd = -1;
> > > + if (use_landlock) {
> > > + struct landlock_ruleset_attr attr = {
> > > + .handled_access_fs =
> > > + 0xffff, /* All FS access rights as of 2026-01 */
> > > + };
> > > + ruleset_fd = syscall(SYS_landlock_create_ruleset, &attr,
> > > + sizeof(attr), 0U);
> > > + if (ruleset_fd < 0)
> > > + err(1, "landlock_create_ruleset");
> > > + }
> > > +
> > > + current = open(".", O_PATH);
> > > + if (current < 0)
> > > + err(1, "open(.)");
> > > +
> > > + while (depth--) {
> > > + if (use_landlock) {
> > > + struct landlock_path_beneath_attr attr = {
> > > + .allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> > > + .parent_fd = current,
> > > + };
> > > + if (syscall(SYS_landlock_add_rule, ruleset_fd,
> > > + LANDLOCK_RULE_PATH_BENEATH, &attr, 0) < 0)
> > > + err(1, "landlock_add_rule");
> > > + }
> > > +
> > > + if (mkdirat(current, path, 0700) < 0)
> > > + err(1, "mkdirat(%s)", path);
> >
> > We should have a loop to build the directories, then start the timer and
> > have another loop to add Landlock rules.
>
> I have to politely push back on this; the granularity of time
> measurement is not high enough and the measurement below only works
> because we repeat it 100000 times. This is not the case when we
> construct a Landlock ruleset, and it would IMHO be weird to build the
> ruleset multiple times as well. It feels like this would better be
> measured in a separate benchmark.
>
> Adding a rule is an operation whose runtime does not depend on the
> depth of the nested directories, so such a separate benchmark would
> then also be simpler and wouldn't need to construct such a deeply
> nested hierarchy.
OK. Please add this explanation in a comment.
>
>
> > > + printf("*** Benchmark ***\n");
> >
> > We should probably use ksft_*() helpers in main (see
> > seccomp_benchmark.c).
>
> Among the benchmarks, the seccomp benchmark is the one exception in
> that it uses these ksft_*() helpers, and it's not clear to me that it
> has any benefit. These helpers are for producing TAP-formatted
> output, and assume that there will be individual test cases with
> success/failure results, which is not the case here. The seccomp test
> uses approximate assertions about the expected timing of operations
> (+-10%), but I don't think we can easily do that in our case.
>
> I would therefore prefer to use a normal textual output format,
> similar to the other benchmarks in tools/testing/kselftests.
OK
>
>
> > > + printf("%zu dirs, %zu iterations, %s landlock\n", num_subdirs,
> > > + num_iterations, use_landlock ? "with" : "without");
> > > +
> > > + if (times(&start_time) == -1)
> > > + err(1, "times");
> > > +
> > > + current = build_directory(num_subdirs, use_landlock);
> > > +
> > > + for (int i = 0; i < num_iterations; i++) {
> > > + fd = openat(current, ".", O_DIRECTORY);
> >
> > We can use AT_EMPTY_PATH (with an empty path) instead of "."
> > I guess the benchmark should not change, but better to check again.
>
> This had to change anyway; now that I added cleanup of the created
> directories, I had to use another operation here that would trigger
> the path walk (file open for creation). Opening directories and
> removing directories both need to continue working so that we can
> later remove the directories. (See discussion below.)
>
>
> > > + if (fd != -1) {
> > > + if (use_landlock)
> > > + errx(1, "openat succeeded, expected error");
> > > +
> > > + close(fd);
> > > + }
> > > + }
> > > +
> > > + if (times(&end_time) == -1)
> > > + err(1, "times");
> >
> > The created directories should be removed here (setup and teardown).
>
> Done.
>
> Minor implementation remark: This is also done with explicit loops
> that use openat() to walk the directory tree with file descriptors and
> then unlinkat(fd, "d", ...). At this nesting depth, the paths don't
> fit into PATH_MAX any more and relative dirfds are the only way to do
> that AFAIK. (The directory walk function nftw(3) also breaks down
> FWIW, because it uses long paths relative to cwd.)
>
> –Günther
>
^ permalink raw reply
* Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Amir Goldstein @ 2026-02-06 13:35 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <177034031005.16766.246184445940612287@noble.neil.brown.name>
On Fri, Feb 6, 2026 at 2:11 AM NeilBrown <neilb@ownmail.net> wrote:
>
> On Thu, 05 Feb 2026, Amir Goldstein wrote:
> > On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> > >
> > > From: NeilBrown <neil@brown.name>
> > >
> > > When ovl_create_real() is used to create a file on the upper filesystem
> > > it needs to return the resulting dentry - positive and hashed.
> > > It is usually the case the that dentry passed to the create function
> > > (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> > > filesystem may unhash that dentry forcing a repeat lookup next time the
> > > name is wanted.
> > >
> > > So ovl_create_real() must be (and is) aware of this and prepared to
> > > perform that lookup to get a hash positive dentry.
> > >
> > > This is currently done under that same directory lock that provided
> > > exclusion for the create. Proposed changes to locking will make this
> > > not possible - as the name, rather than the directory, will be locked.
> > > The new APIs provided for lookup and locking do not and cannot support
> > > this pattern.
> > >
> > > The lock isn't needed. ovl_create_real() can drop the lock and then get
> > > a new lock for the lookup - then check that the lookup returned the
> > > correct inode. In a well-behaved configuration where the upper
> > > filesystem is not being modified by a third party, this will always work
> > > reliably, and if there are separate modification it will fail cleanly.
> > >
> > > So change ovl_create_real() to drop the lock and call
> > > ovl_start_creating_upper() to find the correct dentry. Note that
> > > start_creating doesn't fail if the name already exists.
> > >
> > > This removes the only remaining use of ovl_lookup_upper, so it is
> > > removed.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > > fs/overlayfs/dir.c | 24 ++++++++++++++++++------
> > > fs/overlayfs/overlayfs.h | 7 -------
> > > 2 files changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > > index ff3dbd1ca61f..ec08904d084d 100644
> > > --- a/fs/overlayfs/dir.c
> > > +++ b/fs/overlayfs/dir.c
> > > @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > > err = -EIO;
> > > } else if (d_unhashed(newdentry)) {
> > > struct dentry *d;
> > > + struct name_snapshot name;
> > > /*
> > > * Some filesystems (i.e. casefolded) may return an unhashed
> > > - * negative dentry from the ovl_lookup_upper() call before
> > > + * negative dentry from the ovl_start_creating_upper() call before
> > > * ovl_create_real().
> >
> >
> > According to the new locking rules, if the hashed dentry itself is
> > the synchronization object, is it going to be allowed to
> > filesystem to unhash the dentry while the dentry still in the
> > "creating" scope? It is hard for me to wrap my head around this.
>
> It can be confusing....
>
> It will be important for the name the remain locked (and hashed) until
> the operation (create, remove, rename) either succeeds or fails. So
> leaving a dentry unhashed will be OK providing a subsequent lookup will
> also succeed or fail in the same way. The caller must be able to use
> the dentry to access the object (i.e. the inode) on success, but they
> is nothing in POSIX that requires that the object still has any
> particular name.
>
> >
> > Or do we need this here because some filesystems (casefold in
> > particular) are not going to support parallel creations?
>
> There is no reason that a casefolding filesystem would not support parallel
> ops. And it isn't just casefolding that acts like this. At least one of
> the special filesystems (tracefs maybe) always unhashes on create. You
> only ever get a hashed positive dentry as a result of lookup.
> (overlayfs would never see this case of course).
>
> >
> > > * In that case, lookup again after making the newdentry
> > > * positive, so ovl_create_upper() always returns a hashed
> > > * positive dentry.
> > > + * As we have to drop the lock before the lookup a race
> > > + * could result in a lookup failure. In that case we return
> > > + * an error.
> > > */
> > > - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > > - newdentry->d_name.len);
> > > - dput(newdentry);
> > > - if (IS_ERR_OR_NULL(d))
> > > + take_dentry_name_snapshot(&name, newdentry);
> > > + end_creating_keep(newdentry);
> > > + d = ovl_start_creating_upper(ofs, parent, &name.name);
> > > + release_dentry_name_snapshot(&name);
> >
> > OK. not saying no to this (yet) but I have to admit that it is pretty
> > ugly that the callers of ovl_create_real() want to create a specific
> > stable name, which is could be passed in as const char *name
> > and yet we end up doing this weird dance here just to keep the name
> > from newdentry.
>
> There are three callers of ovl_create_real()
>
> ovl_lookup_or_create() does have a "const char *name".
> ovl_create_upper() has a stable dentry from which it can copy a QSTR
> ovl_create_temp() would need some sort of dance to keep hold of the
> temporary name that was allocated.
>
> If it weren't for ovl_create_temp() I would agree with you.
>
> Though we could have the three callers of ovl_start_creating_temp() pass a
> "char name[OVL_TEMPNAME_SIZE]" in, then ovl_create_temp() would have
> easy access.
> I could do that if you like.
Yes, considering that two of the callers are from the same function
(ovl_whiteout()) I think that would end up looking nicer.
Thanks,
Amir.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox