Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-20687

excessive smatch warnings for container_of_safe use

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Minor Minor
    • Lustre 2.18.0
    • Lustre 2.18.0
    • None
    • 3
    • 9223372036854775807

      Ai generated report following:

       h1. Summary

      lustre: container_of_safe() in the downcast accessors makes every caller a
      potential ERR_PTR dereference

      Description

      A smatch scan of current master (kernel modules only, dd48e6b096) produces
      662 reports outside the out-of-scope generated ldiskfs/ tree. *494 of them
      – three quarters of the whole scan – are one class*, and they all come from a
      single macro.

      The ~46 inline accessors that downcast an lu_device or lu_object (or a
      cl_, dt_ or md_ layer of one) to its containing per-module
      structure – lu2dt_dev(), lu2cl(), mdt_obj(), lu2lod_dev() and
      the rest – are built on container_of_safe()
      (include/linux/libcfs/libcfs.h:79):

      #define container_of_safe(ptr, type, member)		\
      	(IS_ERR_OR_NULL(ptr) ? ERR_CAST(ptr)		\
      	 : container_of(ptr, type, member))
      

      so each of them returns a NULL or an ERR_PTR argument unchanged instead
      of doing the subtraction. A downcast that can hand back an error pointer makes
      every caller a potential error-pointer dereference, which is exactly what
      smatch reports: *439 "dereferencing possible ERR_PTR()" errors plus 39 "passing
      zero to ERR_CAST" warnings*, spread across essentially every server and client
      module.

      That pass-through was never a deliberate contract. The accessors were
      mass-converted from the lustre-local container_of0() by 200d442378
      ("LU-6142 lustre: convert use of container_of0 in include/") and 46f1fc6c1b
      ("LU-6142 lustre: convert some container_of to *_safe"), whose commit messages
      say the arguments "cannot be determined from local inspection" to be valid –
      the old behaviour was simply preserved rather than chosen. The tree has never
      been consistent about it either: the lov, lovsub and lod_obj()
      accessors have always used plain container_of() for the identical downcast,
      and oap2osc() / oap2osc_page() are the same cast written both ways.

      The cost is not only the noise. Code has since been added purely to placate
      the pass-through:

      • the IS_ERR() tests in the llog code from f74ef5903a ("LU-14291 llog: test if dt_device not an ERR_PTR")
      • the IS_ERR_OR_NULL() in lod_comp_prep_create() from c53abca9f9 ("LU-19852 lod: raidset aware stripe allocator")
      • the safety checks in mdt_lproc.c from 117be08e58 ("LU-18961 mdt: add safety checks in MDT_BOOL_RW_ATTR")

      and it hides real defects in the noise: five genuine error-pointer bugs were
      found underneath this class during triage and are fixed by the first five
      patches of this series.

            wc-triage WC Triage
            green Oleg Drokin
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: