On Fri, Mar 24, 2017 at 4:56 PM, Alexander Bluhm <[email protected]>
wrote:
> Hi,
>
> There is a race in dosendsyslog() which resulted in a crash on a
> 5.9 system. sosend(syslogf->f_data, ...) was called with a NULL
> pointer. So syslogf is not NULL, f_data is NULL and f_count is 1.
>
> The file structure is ref counted, but the global variable syslogf
> is not protected. So it may change during sleep and dosendsyslog()
> possibly uses a different socket at each access.
>
> Solution is to access syslogf ony once, use a local copy, and do
> the ref counting there.
>
I'm sending this from the gmail web interface, so formatting may be screwed
up. Apologies in advance.
The patch is somewhat incorrect, although from what I checked it happens
to generate the expected outcome in terms of assembly (the global pointer
read only once and then a local copy accessed several times). You either
need a linux-like READ_ONCE macros which casts through a volatile pointer
or mark the global as volatile to prevent the compiler from issuing
multiple reads in the future.
Remaining ses sof syslogf are also super fishy:
1. logclose
if (syslogf)
FRELE(syslogf, p);
syslogf = NULL;
If this results in fdrop I presume it can block. But if that happens, the
global points to a defunct object.
2. logioctl -> LIOCSFD
if (syslogf)
FRELE(syslogf, p);
syslogf = fp;
This one will give double free and ref leaks.
Suggested fix is to xchg the pointer. While it is more expensive than
necessary for this case, it does not pose a problem. Also xchg is
blatantly obvious, while manual replacement would require explicit memory
barriers to be placed here.
