linux-kernel - Re: drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20201116104413.GB18329@kadam>
Date:   Mon, 16 Nov 2020 13:44:13 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Chris Wilson <chris@...is-wilson.co.uk>
Cc:     kbuild@...ts.01.org, lkp@...el.com, kbuild-all@...ts.01.org,
        linux-kernel@...r.kernel.org,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...el.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Subject: Re: drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59
 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig
 line 59)

On Mon, Nov 16, 2020 at 10:15:04AM +0000, Chris Wilson wrote:
> Quoting Dan Carpenter (2020-11-16 10:08:38)
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git  master
> > head:   0062442ecfef0d82cd69e3e600d5006357f8d8e4
> > commit: 27a5dcfe73f4b696b3de8c23a560199bb1c193a4 drm/i915/gem: Remove disordered per-file request list for throttling
> > config: i386-randconfig-m021-20201115 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> > 
> > smatch warnings:
> > drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)
> > 
> > vim +59 drivers/gpu/drm/i915/gem/i915_gem_throttle.c
> > 
> >     35  int
> >     36  i915_gem_throttle_ioctl(struct drm_device *dev, void *data,
> >     37                          struct drm_file *file)
> >     38  {
> >     39          const unsigned long recent_enough = jiffies - DRM_I915_THROTTLE_JIFFIES;
> >     40          struct drm_i915_file_private *file_priv = file->driver_priv;
> >     41          struct i915_gem_context *ctx;
> >     42          unsigned long idx;
> >     43          long ret;
> >     44  
> >     45          /* ABI: return -EIO if already wedged */
> >     46          ret = intel_gt_terminally_wedged(&to_i915(dev)->gt);
> >     47          if (ret)
> >     48                  return ret;
> >     49  
> >     50          rcu_read_lock();
> >     51          xa_for_each(&file_priv->context_xa, idx, ctx) {
> >     52                  struct i915_gem_engines_iter it;
> >     53                  struct intel_context *ce;
> >     54  
> >     55                  if (!kref_get_unless_zero(&ctx->ref))
> >     56                          continue;
> >     57                  rcu_read_unlock();
> >     58  
> >     59                  for_each_gem_engine(ce,
> >     60                                      i915_gem_context_lock_engines(ctx),
> >                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > I don't understand why this takes the lock every iteration through the
> > loop
> 
> It doesn't.
> 
> static inline struct i915_gem_engines *
> i915_gem_context_lock_engines(struct i915_gem_context *ctx)
>         __acquires(&ctx->engines_mutex)
> {
>         mutex_lock(&ctx->engines_mutex);
>         return i915_gem_context_engines(ctx);
> }
> 
> static inline void
> i915_gem_context_unlock_engines(struct i915_gem_context *ctx)
>         __releases(&ctx->engines_mutex)
> {
>         mutex_unlock(&ctx->engines_mutex);
> }
> 
> with the i915_gem_engines stored as a local in the iterator at the start
> of the for loop.

Yeah...  But that's true enough.  But what I think is actually causing
the static checker warning are the continues.

    52          xa_for_each(&file_priv->context_xa, idx, ctx) {
    53                  struct i915_gem_engines_iter it;
    54                  struct intel_context *ce;
    55  
    56                  if (!kref_get_unless_zero(&ctx->ref))
    57                          continue;
    58                  rcu_read_unlock();
    59  
    60                  for_each_gem_engine(ce,
    61                                      i915_gem_context_lock_engines(ctx),
    62                                      it) {
    63                          struct i915_request *rq, *target = NULL;
    64  
    65                          if (!ce->timeline)
    66                                  continue;
                                        ^^^^^^^^^
This continue is for the inside loop, so "ctx" isn't iterated.  There
is another continue as well later in the loop.  Potentially they could
be replaced with breaks?

    67  
    68                          mutex_lock(&ce->timeline->mutex);
    69                          list_for_each_entry_reverse(rq,
    70                                                      &ce->timeline->requests,
    71                                                      link) {
    72                                  if (i915_request_completed(rq))

regards,
dan carpenter