Bug #11556
closedip_attr.c functions need to not dereference conn_ixa directly after lock drop
100%
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).
Updated by Dan McDonald almost 4 years 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.
Updated by Dan McDonald almost 4 years 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.
Updated by Electric Monk almost 4 years 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>