linux-kernel - Re: [PATCH v2 1/2] locking: Introduce __cleanup_

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgt=cq_fU33DCv0=xD30sH8eOGHsEQRJQi=2vtrWt7oPQ@mail.gmail.com>
Date:   Fri, 26 May 2023 14:54:40 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     keescook@...omium.org, gregkh@...uxfoundation.org,
        pbonzini@...hat.com, linux-kernel@...r.kernel.org,
        ojeda@...nel.org, ndesaulniers@...gle.com, mingo@...hat.com,
        will@...nel.org, longman@...hat.com, boqun.feng@...il.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vschneid@...hat.com,
        paulmck@...nel.org, frederic@...nel.org, quic_neeraju@...cinc.com,
        joel@...lfernandes.org, josh@...htriplett.org,
        mathieu.desnoyers@...icios.com, jiangshanlai@...il.com,
        rcu@...r.kernel.org, tj@...nel.org, tglx@...utronix.de
Subject: Re: [PATCH v2 1/2] locking: Introduce __cleanup__ based guards

On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <peterz@...radead.org> wrote:
>> > +                                                                             \
> > +static inline void ptr_guard_##_type##_cleanup(_Type **_ptr)                 \
> > +{                                                                            \
> > +     _Type *_G = *_ptr;                                                      \
> > +     if (_G)                                                                 \
> > +             _Put(_G);                                                       \
> > +}
> > +
> > +#define ptr_guard(_type, _name)                                                      \
> > +     ptr_guard_##_type##_t _name __cleanup(ptr_guard_##_type##_cleanup)
> > +
>
> Ha, and then I wanted to create an fdput() guard... :/

I have to say, I'm not super-convinced about the macros to create
these guard functions and types.

I suspect that it would be better to literally just declare the guard
types directly.  For the fdget idiom, you really just want the guard
type to *be* the 'struct fd'.

And I think you made those macros just make too many assumptions.

It's not just "struct fd", for example.  If you want to do things like
wrap 'local_irq_save', again it's not a pointer, the context is just
'unsigned long irqflags'.

And I really don't think those type-creation macros buy you anything.

What's wrong with just writing it out:

    typedef struct fd guard_fdget_type_t;
    static inline struct fd guard_fdget_init(int fd)
    { return fdget(fd); }
    static inline void guard_fdget_exit(struct fd fd)
    { fdput(fd); }

and

    typedef struct mutex *guard_mutex_type_t;
    static inline struct mutex *guard_mutex_init(struct mutex *mutex)
    { mutex_lock(mutex); return mutex; }
    static inline void guard_mutex_exit(struct mutex *mutex)
    { mutex_unlock(mutex); }

I don't think the latter is in the *least* less readable than doing

    DEFINE_LOCK_GUARD_1(mutex, struct mutex,
                mutex_lock(_G->lock),
                mutex_unlock(_G->lock))

which is this magic macro that creates those, and makes them
completely ungreppable - and makes the type system very inflexible.

So I really think that it would be a lot easier to write out the
wrappers explicitly for the few types that really want this.

I dunno.

Once again, I have written example code in my MUA that I have not
tested AT ALL, and that may be fundamentally broken for some very
obvious reason, and I'm just too stupid to see it.

             Linus