Context: we have quite an involved config that was built on 2.x, and we used to make use of the sql_log module, with a locally-constructed external process which follows the file and relays to our SQL service. That external process has a very carefully constructed open/lock/rename scheme designed to cooperate with how 2.x worked. That module has gone away on 3.x and we've been using an instance of sql with the null driver and "logfile" option set, which I was under the impression used the same locking scheme. We don't use the detail writer/reader construction because we're using this in the "postauth" section, for logging of auths on devices which don't do useful accounting (layer2 switches doing mac-based VLANs). Having finally got round to serious porting to 3.x, we're seeing corruption of this file on 3.0.15 and I am suspicious of the exfile locking functions which I see are now being used for this. Specifically: https://github.com/FreeRADIUS/freeradius-server/blob/v3.0.x/src/main/exfile.... ...seems to perform an fstat/re-open-if-unlinked check, but this happens *after* any fcntl locking has been done, and there's no attempt to re-lock the file on this re-open. So, if this codepath gets triggered, the file is now held and written to without locking, when it should be locked. Bug? More generally, I've re-read the exfile code a couple of times and I am suspicious of the basic construction. What performance problem is the open/dup caching setup intended to solve? Put another way, why not open/close the file using the traditional posix APIs on each run? Regards, Phil
On Jul 25, 2017, at 7:34 AM, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
Having finally got round to serious porting to 3.x, we're seeing corruption of this file on 3.0.15 and I am suspicious of the exfile locking functions which I see are now being used for this. Specifically:
https://github.com/FreeRADIUS/freeradius-server/blob/v3.0.x/src/main/exfile....
...seems to perform an fstat/re-open-if-unlinked check, but this happens *after* any fcntl locking has been done, and there's no attempt to re-lock the file on this re-open. So, if this codepath gets triggered, the file is now held and written to without locking, when it should be locked. Bug?
Possibly. I'll take a look.
More generally, I've re-read the exfile code a couple of times and I am suspicious of the basic construction. What performance problem is the open/dup caching setup intended to solve? Put another way, why not open/close the file using the traditional posix APIs on each run?
Because the traditional POSIX APIs are utter shit. If a process opens a file multiple times, and does fcntl() lock on one file descriptor, closing any OTHER file descriptor on that file causes the lock to go away. The only way to deal with this correctly is to add a wrapper layer around the open / lock / close, which uses mutexes for mutual exclusion and locking. It still has to use fcntl() locking so that *other* processes trying to open the file get locked out (or not) as needed. i.e. you can't rely on fcntl() locks within a process. They are *only* for inter-process locking. Alan DeKok.
On 25/07/17 12:48, Alan DeKok wrote:
Because the traditional POSIX APIs are utter shit.
Indeed.
If a process opens a file multiple times, and does fcntl() lock on one file descriptor, closing any OTHER file descriptor on that file causes the lock to go away.
Sure, that's well documented (as opposed to well understood ;o), and the mutex use is fine (necessary, as you've noted). What I don't understand is why the code open()s to one FD but returns a dup()d FD to the caller. This seems to accomplish nothing - you're still paying for a context switch on the dup() and it's frankly hard for me to believe that saving the open() is worth the extra code complexity, but I'm assuming there's a reason. Unless I'm missing something, the mutex around the exfile isn't dependent on the dup()ing of the FD?
On Jul 25, 2017, at 8:29 AM, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
What I don't understand is why the code open()s to one FD but returns a dup()d FD to the caller. This seems to accomplish nothing - you're still paying for a context switch on the dup() and it's frankly hard for me to believe that saving the open() is worth the extra code complexity, but I'm assuming there's a reason.
Hmm... I thought so, but perhaps not...
Unless I'm missing something, the mutex around the exfile isn't dependent on the dup()ing of the FD?
It returns from exfile_open() with the mutex held. So the dup() is done while the mutex is locked. The exfile_close() releases the mutex. Alan DeKok.
participants (2)
-
Alan DeKok -
Phil Mayers