mm = current->mm;
>> + } else {
>> + /* this can deadlock outside the fork/clone usage (see
>> above) */
>> + mm = get_task_mm(tsk);
>> + if (!mm)
>> + return 0;
>> + }
>> + exe_file = get_mm_exe_file(mm);
>> + mmput(mm);
>> if (!exe_file)
>> return 0;
>> ino = file_inode(exe_file)->i_ino;
Assuming concerns expressed above are moot, I would hide the logic in
audit_get_task_exe_file or similar.
--
Mateusz Guzik
On 10/19/23, Paul Moore wrote:
> On Wed, Oct 18, 2023 at 8:22 PM Mateusz Guzik wrote:
>> On 10/19/23, Paul Moore wrote:
>> >> The get_task_exe_file() function locks the given task with task_lock()
>> >> which when used inside audit_exe_compare() can caus
On 10/19/23, Mateusz Guzik wrote:
> On 10/19/23, Paul Moore wrote:
>> On Wed, Oct 18, 2023 at 8:22 PM Mateusz Guzik wrote:
>>> On 10/19/23, Paul Moore wrote:
>>> >> The get_task_exe_file() function locks the given task with
>>> >> task_lock()
ing rlimits is cheaper. I'm going to create a new thread about it
later.
--
Mateusz Guzik
On 10/19/23, Paul Moore wrote:
> On Thu, Oct 19, 2023 at 10:52 AM Mateusz Guzik wrote:
>> On 10/19/23, Paul Moore wrote:
>> > Thinking about it a bit more this morning, I think we can safely
>> > ignore the non-@current case in audit_exe_compare() as the whole point
&
On 10/19/23, Paul Moore wrote:
> On Thu, Oct 19, 2023 at 12:56 PM Mateusz Guzik wrote:
>> On 10/19/23, Paul Moore wrote:
>> > On Thu, Oct 19, 2023 at 10:52 AM Mateusz Guzik
>> > wrote:
>> >> On 10/19/23, Paul Moore wrote:
>> >> > Thi
->mm has to be set.
Although I do find it plausible there maybe a corner case during
kernel bootstrap and it may be that code can land here with that
state, but I can't be arsed to check.
Given that stock code has an unintentional property of handling empty
mm and this is a bugfix, I am definitely not going to protest adding a
check. But I would WARN_ONCE it though.
>> > if (!exe_file)
>> > return 0;
>> > ino = file_inode(exe_file)->i_ino;
>> > dev = file_inode(exe_file)->i_sb->s_dev;
>> > fput(exe_file);
>> > +
>> > return audit_mark_compare(mark, ino, dev);
>> > }
>
> --
> paul-moore.com
>
--
Mateusz Guzik
This does fix the bug, and with the caveat I can't vouch for ignoring
non-current in the routine:
Reviewed-by: Mateusz Guzik
Thanks for fixing the bug. Pretty weird it took this long for it to surface.
On 10/24/23, Paul Moore wrote:
> On Tue, Oct 24, 2023 at 2:39 PM Paul Moor
On 11/14/23, Artem Savkov wrote:
> On Tue, Oct 24, 2023 at 07:59:18PM +0200, Mateusz Guzik wrote:
>> For the thread to start executing ->mm has to be set.
>>
>> Although I do find it plausible there maybe a corner case during
>> kernel bootstrap and it may be tha
On 11/14/23, Paul Moore wrote:
> On Tue, Nov 14, 2023 at 5:33 AM Mateusz Guzik wrote:
>> On 11/14/23, Artem Savkov wrote:
>> > On Tue, Oct 24, 2023 at 07:59:18PM +0200, Mateusz Guzik wrote:
>> >> For the thread to start executing ->mm has to be set.
>> &
On Thu, Feb 6, 2025 at 9:24 PM Jeff Layton wrote:
>
> On Thu, 2025-02-06 at 20:07 +0100, Mateusz Guzik wrote:
> > You added it in:
> > commit 7ac86265dc8f665cc49d6e60a125e608cd2fca14
> > Author: Jeff Layton
> > Date: Wed Oct 10 15:25:28 2012 -0400
> >
> &
On Thu, Feb 6, 2025 at 9:34 PM Mateusz Guzik wrote:
>
> On Thu, Feb 6, 2025 at 9:24 PM Jeff Layton wrote:
> >
> > On Thu, 2025-02-06 at 20:07 +0100, Mateusz Guzik wrote:
> > > You added it in:
> > > commit 7ac86265dc8f665cc49d6e60a125e608cd2fca14
> > &g
tions from
disabled to enabled -- will they suddenly start looking here? Should
this test for audit_enabled, audit_dummy_context() or something else?
I did not want to bother analyzing this.
I'll note though this would be an optimization on top of the current
code, so I don't think it *blocks* the patch.
--
Mateusz Guzik
ame object and having refname()
and putname() detect if another thread is trying to modify them.
Benchmarked on Sapphire Rapids issuing access() (ops/s):
before: 5106246
after: 5269678 (+3%)
Signed-off-by: Mateusz Guzik
---
this can be split into 2 patches (refname, initname vs the atomic
chang
On Fri, Mar 7, 2025 at 5:32 PM Jens Axboe wrote:
>
> On 3/7/25 9:25 AM, Mateusz Guzik wrote:
> > On Fri, Mar 7, 2025 at 5:18?PM Jens Axboe wrote:
> >>
> >>> +static inline void makeatomicname(struct filename *name)
> >>>
On Fri, Mar 7, 2025 at 5:38 PM Jens Axboe wrote:
>
> On 3/7/25 9:35 AM, Mateusz Guzik wrote:
> > Since you volunteered to sort this out, I'll be happy to wait.
>
> I'll take a look start next week, don't think it should be too bad. You
> already did 90% of t
On Fri, Mar 7, 2025 at 5:42 PM Al Viro wrote:
>
> On Fri, Mar 07, 2025 at 05:11:55PM +0100, Mateusz Guzik wrote:
> > Atomics are only needed for a combination of io_uring and audit.
> >
> > Regular file access (even with audit) gets around fine without them.
> &g
On Fri, Mar 7, 2025 at 5:26 PM Matthew Wilcox wrote:
>
> On Fri, Mar 07, 2025 at 05:11:55PM +0100, Mateusz Guzik wrote:
> > +++ b/include/linux/fs.h
> > @@ -2765,11 +2765,19 @@ struct audit_names;
> > struct filename {
> > const char *name
On Fri, Mar 7, 2025 at 5:44 PM Mateusz Guzik wrote:
>
> On Fri, Mar 7, 2025 at 5:42 PM Al Viro wrote:
> > Not a good way to handle that, IMO.
> >
> > Atomics do hurt there, but they are only plastering over the real
> > problem - names formed in one thread, inserte
/s):
before: 5106246
after: 5269678 (+3%)
Link 1:
https://lore.kernel.org/linux-fsdevel/20250307161155.760949-1-mjgu...@gmail.com/
Link 2: https://lore.kernel.org/linux-fsdevel/20250307164216.GI2023217@ZenIV/
Signed-off-by: Mateusz Guzik
---
This is an alternative to the patch I linked above
20 matches
Mail list logo
