On Fri, Mar 24, 2017 at 4:56 PM, Alexander Bluhm <alexander.bl...@gmx.net> 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.