Project

General

Profile

Bug #11556

ip_attr.c functions need to not dereference conn_ixa directly after lock drop

Added by Dan McDonald 3 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
networking
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Hard
Tags:

Description

There's an antipattern in two places in ip_attr.c.

        mutex_enter(&conn->conn_lock);
        /* manipulate lock-protected conn_ixa pointer, even refholding it... */
        IXA_REFHOLD(conn->conn_ixa);
        mutex_exit(&conn->conn_lock);
        IXA_REFRELE(conn->conn_ixa);  /* XXX KEBE SAYS THIS IS WRONG! */

After conn_lock is dropped, conn_ixa MAY be replaced. You may be refrele-ing a different conn_ixa than you refheld inside the mutex!!!

A better pattern is:

        ip_xmit_attr_t *oldixa;

        mutex_enter(&conn->conn_lock);
        /* manipulate lock-protected conn_ixa pointer, even refholding it... */
        oldixa = conn->conn_ixa;
        IXA_REFHOLD(oldixa);
        mutex_exit(&conn->conn_lock);
        IXA_REFRELE(oldixa);  /* XXX KEBE SAYS MUCH BETTER! */

There is other TCP/IP code that does ixa_refrele(conn->conn_ixa), but they are all under squeue exclusivity OR part of conn_t destruction post-detachment.

This bug manifests like a use-after-free on non-DEBUG kernels and blows an ASSERT in ixa_inactive (refcnt is not necessarily 1).

History

#1

Updated by Dan McDonald 3 months ago

Not mentioned eariler, the IXA_REFRELE macro evaluates its argument multiple times. This exacerbates the problem because connp->conn_ixa can change in the middle of the macro. IXA_REF{hold,rele} must not have an argument that evaluates multiple times. It's one reason why these aren't used in header files, and instead we have the lowercased function versions.

#2

Updated by Dan McDonald 3 months ago

Bug can be reproduced by an application that uses multiple threads that each call sendmsg(3xnet) concurrently over a single socket on a loaded system. Each distinct sendmsg(3xnet) call references-and-clones the conn_ixa. The go network tests are effective here. I ran this on a DEBUG OmniOS to trigger the bug (assertion failure). Then I applied the fix, and tried again with no panics on the post-fix run.

#3

Updated by Electric Monk 3 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 79940ff6ac581ff9431c474dcfa18c78f1cb7a50

commit  79940ff6ac581ff9431c474dcfa18c78f1cb7a50
Author: Dan McDonald <danmcd@joyent.com>
Date:   2019-08-15T18:35:49.000Z

    11556 ip_attr.c functions need to not dereference conn_ixa directly after lock drop
    Reviewed by: Jason King <jbk@joyent.com>
    Reviewed by: Mike Gerdts <mgerdts@joyent.com>
    Reviewed by: Andy Fiddaman <andy@omniosce.org>
    Approved by: Gordon Ross <gwr@nexenta.com>

Also available in: Atom PDF