drivers/tty/tty_io.c: callers of tty_write_lock check return value with <0

[bulwahn]

drivers/tty/tty_io.c:979:2: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
        tty_write_unlock(tty);
        ^
drivers/tty/tty_io.c:1098:2: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
        tty_write_unlock(tty);
        ^
drivers/tty/tty_io.c:2394:3: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
                tty_write_unlock(tty);
                ^

[bulwahn]

I hoped that rewriting the checks as follows would make clang understand:

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b113d64e89e5..2507261dbfad 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -908,7 +908,7 @@ static inline ssize_t do_tty_write(
        unsigned int chunk;

        ret = tty_write_lock(tty, file->f_flags & O_NDELAY);
-       if (ret < 0)
+       if (!ret)
                return ret;

        /*
@@ -1085,7 +1085,7 @@ int tty_send_xchar(struct tty_struct *tty, char ch)
                return 0;
        }

-       if (tty_write_lock(tty, 0) < 0)
+       if (!tty_write_lock(tty, 0))
                return -ERESTARTSYS;

        down_read(&tty->termios_rwsem);
@@ -2382,7 +2382,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
                retval = tty->ops->break_ctl(tty, duration);
        else {
                /* Do the work ourselves */
-               if (tty_write_lock(tty, 0) < 0)
+               if (!tty_write_lock(tty, 0))
                        return -EINTR;
                retval = tty->ops->break_ctl(tty, -1);
                if (retval)

But, it just made even complain more:

drivers/tty/tty_io.c:979:2: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
        tty_write_unlock(tty);
        ^
drivers/tty/tty_io.c:981:1: warning: mutex 'tty->atomic_write_lock' is not held on every path through here [-Wthread-safety-analysis]
}
^
drivers/tty/tty_io.c:910:8: note: mutex acquired here
        ret = tty_write_lock(tty, file->f_flags & O_NDELAY);
              ^
drivers/tty/tty_io.c:1098:2: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
        tty_write_unlock(tty);
        ^
drivers/tty/tty_io.c:1100:1: warning: mutex 'tty->atomic_write_lock' is not held on every path through here [-Wthread-safety-analysis]
}
^
drivers/tty/tty_io.c:1088:7: note: mutex acquired here
        if (!tty_write_lock(tty, 0))
             ^
drivers/tty/tty_io.c:2394:3: warning: releasing mutex 'tty->atomic_write_lock' that was not held [-Wthread-safety-analysis]
                tty_write_unlock(tty);
                ^
drivers/tty/tty_io.c:2399:1: warning: mutex 'tty->atomic_write_lock' is not held on every path through here [-Wthread-safety-analysis]
}
^
drivers/tty/tty_io.c:2385:8: note: mutex acquired here
                if (!tty_write_lock(tty, 0))
                     ^

I am puzzled why clang does not really get this, but it is probably some shortcoming of the whole control flow analysis.

[himanshujha199640]

Just FYI:

  1 struct Mutex {
  2         int mutex;
  3 } __attribute__((capability("mutex")));
  4 
  5 
  6 extern void acquire(struct Mutex *lock) __attribute__((acquire_capability(lock)));
  7 extern void release(struct Mutex *lock) __attribute__((release_capability(lock)));
  8 extern int try_lock(struct Mutex *lock) __attribute__((try_acquire_capability(1, lock)));
  9 
 10 int test(void)
 11 {
 12         int ret;
 13         struct Mutex *lock;
 14  
 15         ret = try_lock(lock);
 16         if(!ret) {
 17                 return 2;
 18         }       
 19  
 20         release(lock);
 21         return 0;
 22 }

It works without any warnings.

I believe we must annotate tty_write_lock function with __try_acquires_mutex as well(assuming mutex_trylock is already annotated with __try_acquires_mutex).

Also, analysis must have reported warning for tty_write_lock for acquiring a lock ?

[bulwahn]

Okay, we need to revisit (maybe, it depends on the clang version). Ideally, I would like to have a set of small examples with __try_acquires attributes and show what clang can analyse properly and what not.

I did annotate tty_write_lock (and the warnings above appeared), but the analysis did not properly continue on those functions mentioned above.