linux-kernel - Re: mm/list_lru.c:514:3-8: WARNING: NULL check before some freeing functions is not needed.

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] [day] [month] [year] [list]

Message-ID: <CAMgjq7DvR2uXi+nMenAFSttrnbeZotM_8V+AgfSzWbJjo6g=qw@mail.gmail.com>
Date: Sun, 29 Dec 2024 17:39:22 +0800
From: Kairui Song <ryncsn@...il.com>
To: kernel test robot <lkp@...el.com>
Cc: oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, 
	Linux Memory Management List <linux-mm@...ck.org>
Subject: Re: mm/list_lru.c:514:3-8: WARNING: NULL check before some freeing
 functions is not needed.

On Sun, Dec 29, 2024 at 10:00 AM kernel test robot <lkp@...el.com> wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   059dd502b263d8a4e2a84809cf1068d6a3905e6f
> commit: 28e98022b31efdb8f1ba310d938cd9b97ededfe4 mm/list_lru: simplify reparenting and initial allocation
> date:   7 weeks ago
> config: i386-randconfig-054-20241229 (https://download.01.org/0day-ci/archive/20241229/202412290924.UTP7GH2Z-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@...el.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202412290924.UTP7GH2Z-lkp@intel.com/
>
> cocci warnings: (new ones prefixed by >>)
> >> mm/list_lru.c:514:3-8: WARNING: NULL check before some freeing functions is not needed.
>
> vim +514 mm/list_lru.c
>
>    470
>    471  int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
>    472                           gfp_t gfp)
>    473  {
>    474          unsigned long flags;
>    475          struct list_lru_memcg *mlru;
>    476          struct mem_cgroup *pos, *parent;
>    477          XA_STATE(xas, &lru->xa, 0);
>    478
>    479          if (!list_lru_memcg_aware(lru) || memcg_list_lru_allocated(memcg, lru))
>    480                  return 0;
>    481
>    482          gfp &= GFP_RECLAIM_MASK;
>    483          /*
>    484           * Because the list_lru can be reparented to the parent cgroup's
>    485           * list_lru, we should make sure that this cgroup and all its
>    486           * ancestors have allocated list_lru_memcg.
>    487           */
>    488          do {
>    489                  /*
>    490                   * Keep finding the farest parent that wasn't populated
>    491                   * until found memcg itself.
>    492                   */
>    493                  pos = memcg;
>    494                  parent = parent_mem_cgroup(pos);
>    495                  while (!memcg_list_lru_allocated(parent, lru)) {
>    496                          pos = parent;
>    497                          parent = parent_mem_cgroup(pos);
>    498                  }
>    499
>    500                  mlru = memcg_init_list_lru_one(gfp);
>    501                  if (!mlru)
>    502                          return -ENOMEM;
>    503                  xas_set(&xas, pos->kmemcg_id);
>    504                  do {
>    505                          xas_lock_irqsave(&xas, flags);
>    506                          if (!xas_load(&xas) && !css_is_dying(&pos->css)) {
>    507                                  xas_store(&xas, mlru);
>    508                                  if (!xas_error(&xas))
>    509                                          mlru = NULL;
>    510                          }
>    511                          xas_unlock_irqrestore(&xas, flags);
>    512                  } while (xas_nomem(&xas, gfp));
>    513                  if (mlru)
>  > 514                          kfree(mlru);

Hmm, does it need a fix? mlru here is most likely always NULL, so in
most cases this should save a function call, a really micro
optimization.

Perhaps a "if (unlikely(mlru))" is better, and silence the bot?

>    515          } while (pos != memcg && !css_is_dying(&pos->css));
>    516
>    517          return xas_error(&xas);
>    518  }
>    519  #else
>    520  static inline void memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
>    521  {
>    522  }
>    523
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>