Bug 234442 - libnetgraph race condition
Summary: libnetgraph race condition
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.2-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-net mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-27 11:25 UTC by Eugene Grosbein
Modified: 2019-01-09 16:58 UTC (History)
5 users (show)

See Also:


Attachments
fight race with mutex (1.55 KB, patch)
2018-12-27 11:25 UTC, Eugene Grosbein
no flags Details | Diff
atomically increment gMsgId (1.40 KB, patch)
2019-01-09 16:55 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Grosbein freebsd_committer 2018-12-27 11:25:47 UTC
Created attachment 200557 [details]
fight race with mutex

Hi!

lib/libnetgraph/msg.c defines static int gMsgId and public functions NgSendMsg() and NgSendAsciiMsg() that both increment gMsgId in racy way in attempt to produce unique id for a request sent over AF_NETGRAPH socket.

For long-lived multi-threaded application like net/mpd5 daemon: first thread can increase gMsgId upto INT_MAX and next moment another thread can increate gMsgId again to become -1. Then it is copied to unsigned msg.header.token and returned as signed integer. This means false error status returned with errno==0 and this breaks workflow of the daemon. I get this problem "in wild" from time to time.

I have very straightforward and naive patch protecting the variable with simple mutex (attached) but it has its penalty for performance.

Usage of atomic operations should be better approach but I'm not familiar with FreeBSD atomic operations. Any help will be appreciated.
Comment 1 Mark Johnston freebsd_committer 2019-01-09 16:55:12 UTC
Created attachment 200960 [details]
atomically increment gMsgId

The attached patch uses C11 atomics in an attempt to fix the problem.  It even compiles and appears to generate the intended code with gcc 4.2.
Comment 2 Mark Johnston freebsd_committer 2019-01-09 16:58:49 UTC
(In reply to Mark Johnston from comment #1)
Note, the patch permits a msgid of 0 instead of starting from 1.  The netgraph(3) man page says that tokens only need to be non-negative, so I think this is fine, but I did not test it.