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

ptlrpc: import reference leaked or double-released on setup failure

XMLWordPrintable

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

      Two import reference bugs on the paths that set a request up before it is
      sent. Both come from the same thing: ptlrpc_request_free() and
      ptlrpc_req_put() are not interchangeable, and the setup paths disagree about
      which one is correct. Related to LU-20050 (Gerrit 64945), which fixes the same
      kind of leak in ptlrpc_connect_import_locked().

      1. The reference is leaked when an idle reconnect fails

      In ptlrpc_request_alloc_internal() the request gets allocated first, and
      __ptlrpc_request_alloc() takes an import reference for it:

      
      

      request->rq_import = class_import_get(imp);
      atomic_inc(&imp->imp_reqs);{code}
      If the import isn't FULL we try an idle reconnect, and the failure path looks like this:

      
      

      if (ptlrpc_reconnect_if_idle(imp) < 0)

      {         atomic_dec(&imp->imp_reqs);         ptlrpc_request_free(request);         return NULL; }

      {code}
      ptlrpc_request_free() only puts the request back on the slab or into the
      pool. It never calls class_import_put(), so the reference taken a few lines
      earlier is just lost. Every failed idle reconnect leaks one, the import is
      never freed, client_obd_cleanup() never runs, and the LDLM namespace kobject
      is left behind in sysfs. The next mount of the same target then fails with
      -EEXIST.

      The easiest way to see it is to set sptlrpc.send_sepol=-1 and break
      l_getsepol: any client whose import has gone idle fails the reconnect, and
      leaks on every RPC that triggers one.

      The fix is to call ptlrpc_req_put() instead — it goes through
      __ptlrpc_free_req(), which already drops the import reference and decrements
      imp_reqs. The open-coded atomic_dec() has to go at the same time, otherwise
      the counter drops twice and the assertion in __ptlrpc_free_req() trips.

      This has been there since 93d20d171c20 ("LU-11128 ptlrpc: new request vs
      disconnect race"), which moved the allocation above the idle check.

      2. The reference is released twice when packing fails

      ptlrpc_request_bufs_pack() has the opposite problem. Its out_free label drops
      the reference, but leaves rq_import pointing at the import:

      
      

      out_free:
              atomic_dec(&imp->imp_reqs);
              class_import_put(imp);

              return rc;{code}
      __ptlrpc_free_req() releases the import whenever rq_import is set. So a
      caller that disposes of the failed request with ptlrpc_req_put() releases the
      same reference a second time.

      Two callers do exactly that today:

      • gss_cli_ctx_fini_rpc() in gss_cli_upcall.c jumps to out_ref on a pack
        error, and out_ref calls ptlrpc_req_put()
      • osp_sync_new_job() in osp_sync.c calls ptlrpc_req_put() on a pack error

      Both over-release the import and decrement imp_reqs twice. The assertion on
      imp_reqs in __ptlrpc_free_req() can fire, and the import can be freed while
      it is still in use.

      The fix is to clear rq_import once the reference has been dropped.
      __ptlrpc_free_req() already skips the release when rq_import is NULL, so
      after this both ptlrpc_request_free() and ptlrpc_req_put() are correct on a
      failed pack, and callers no longer have to know which one to use.

      This one dates to 8e86156c34f3 ("LU-10486 osp: fix request leak on error in
      osp_sync") for the osp caller; the gss caller is older.

      Why they are fixed together

      The two fixes meet in the middle. The first makes a setup path start using
      ptlrpc_req_put(); the second makes ptlrpc_req_put() safe on a setup path
      that has already failed. Together they make ptlrpc_req_put() the one correct
      way to dispose of a request that failed before it was sent, which is what lets
      the callers in mdc drop their second error label (LU-20050, Gerrit 65026).

            hnishida Hiroshi Nishida
            hnishida Hiroshi Nishida
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: