On Wed, 5 Mar 2025 at 10:15, Jiri Slaby <jirislaby@xxxxxxxxxx> wrote:
>
> On 04. 03. 25, 10:21, Marco Elver wrote:
> > Enable capability analysis for drivers/tty/*.
> >
> > This demonstrates a larger conversion to use Clang's capability
> > analysis. The benefit is additional static checking of locking rules,
> > along with better documentation.
> >
> > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Jiri Slaby <jirislaby@xxxxxxxxxx>
> ...
> > --- a/drivers/tty/tty_buffer.c
> > +++ b/drivers/tty/tty_buffer.c
> > @@ -52,10 +52,8 @@
> > */
> > void tty_buffer_lock_exclusive(struct tty_port *port)
> > {
> > - struct tty_bufhead *buf = &port->buf;
> > -
> > - atomic_inc(&buf->priority);
> > - mutex_lock(&buf->lock);
> > + atomic_inc(&port->buf.priority);
> > + mutex_lock(&port->buf.lock);
>
> Here and:
>
> > @@ -73,7 +71,7 @@ void tty_buffer_unlock_exclusive(struct tty_port *port)
> > bool restart = buf->head->commit != buf->head->read;
> >
> > atomic_dec(&buf->priority);
> > - mutex_unlock(&buf->lock);
> > + mutex_unlock(&port->buf.lock);
>
> here, this appears excessive. You are changing code to adapt to one kind
> of static analysis. Adding function annotations is mostly fine, but
> changing code is too much. We don't do that. Fix the analyzer instead.
Right. So the analysis doesn't do alias analysis.
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -167,6 +167,7 @@ static void release_tty(struct tty_struct *tty, int idx);
> > * Locking: none. Must be called after tty is definitely unused
> > */
> > static void free_tty_struct(struct tty_struct *tty)
> > + __capability_unsafe(/* destructor */)
> > {
> > tty_ldisc_deinit(tty);
> > put_device(tty->dev);
> > @@ -965,7 +966,7 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
> > ssize_t ret, written = 0;
> >
> > ret = tty_write_lock(tty, file->f_flags & O_NDELAY);
> > - if (ret < 0)
> > + if (ret)
>
> This change is not documented.
Fair point. This is because the analysis can only deal with
conditional locking when fed into zero/non-zero condition checks.
> > @@ -1154,7 +1155,7 @@ int tty_send_xchar(struct tty_struct *tty, u8 ch)
> > return 0;
> > }
> >
> > - if (tty_write_lock(tty, false) < 0)
> > + if (tty_write_lock(tty, false))
>
> And this one. And more times later.
>
> > --- a/drivers/tty/tty_ldisc.c
> > +++ b/drivers/tty/tty_ldisc.c
> ...
> > +/*
> > + * Note: Capability analysis does not like asymmetric interfaces (above types
> > + * for ref and deref are tty_struct and tty_ldisc respectively -- which are
> > + * dependent, but the compiler cannot figure that out); in this case, work
> > + * around that with this helper which takes an unused @tty argument but tells
> > + * the analysis which lock is released.
> > + */
> > +static inline void __tty_ldisc_deref(struct tty_struct *tty, struct tty_ldisc *ld)
> > + __releases_shared(&tty->ldisc_sem)
> > + __capability_unsafe(/* matches released with tty_ldisc_ref() */)
> > +{
> > + tty_ldisc_deref(ld);
> > +}
>
> You want to invert the __ prefix for these two. tty_ldisc_deref() should
> be kept as the one to be called by everybody.
Ack.
I think in the near term the alias analysis issues + conditional check
of < 0 aren't solvable. Alias analysis being the bigger issue.
Two options:
1. Adding __capability_unsafe to the few functions that you weren't
happy with above.
2. Dropping the whole patch.
I'm inclined to drop the whole patch.
Thanks,
-- Marco
