syslogd: Do not add shutdown sockets to the kqueue
ClosedPublic
Actions

Authored by jfree on Aug 21 2023, 4:25 PM.

Details

Reviewers

markj

Commits

rG94cae4cb339a: syslogd: Do not add shutdown sockets to the kqueue

Summary

If syslogd is in secure mode, all INET sockets are shutdown. Do not
add these to the kqueue because we can't read from them.

Also, remove the listen() call when setting up sockets. Syslogd
exclusively uses SOCK_DGRAM sockets, so calling listen() is useless.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jfree created this revision.Aug 21 2023, 4:25 PM

Herald added a subscriber: imp. · View Herald TranscriptAug 21 2023, 4:25 PM

jfree requested review of this revision.Aug 21 2023, 4:25 PM

jfree added a parent revision: D41459: syslogd: Use pipe to communicate with daemon.

jfree added a child revision: D41463: syslogd: Create syslogd libcasper service.

I don't really understand this change. You wrote, "To fix this, listen() should not be called if syslogd is in secure mode.", but nothing has changed with respect to listen() calls. All that's changed is that we don't call shutdown().

If the problem is that data on unread sockets is thrashing kevent(), why not avoid creating EVFILT_READ events for those sockets?

In D41526#947143, @markj wrote:

I don't really understand this change. You wrote, "To fix this, listen() should not be called if syslogd is in secure mode.", but nothing has changed with respect to listen() calls. All that's changed is that we don't call shutdown().

If the problem is that data on unread sockets is thrashing kevent(), why not avoid creating EVFILT_READ events for those sockets?

Notice the else on line 3751. If we're in secure mode and we have INET sockets, then sl_recv will be set to NULL, else we will listen. No listening happens in secure mode for INET sockets.

In D41526#947144, @jfree wrote:

In D41526#947143, @markj wrote:

I don't really understand this change. You wrote, "To fix this, listen() should not be called if syslogd is in secure mode.", but nothing has changed with respect to listen() calls. All that's changed is that we don't call shutdown().

If the problem is that data on unread sockets is thrashing kevent(), why not avoid creating EVFILT_READ events for those sockets?

Notice the else on line 3751. If we're in secure mode and we have INET sockets, then sl_recv will be set to NULL, else we will listen. No listening happens in secure mode for INET sockets.

Now I'm even more confused. :) hints.ai_socktype = SOCK_DGRAM in the sole caller of socksetup(), so when is it the case that ai->ai_socktype != SOCK_DGRAM?

In D41526#947145, @markj wrote:

In D41526#947144, @jfree wrote:

In D41526#947143, @markj wrote:

I don't really understand this change. You wrote, "To fix this, listen() should not be called if syslogd is in secure mode.", but nothing has changed with respect to listen() calls. All that's changed is that we don't call shutdown().

If the problem is that data on unread sockets is thrashing kevent(), why not avoid creating EVFILT_READ events for those sockets?

Notice the else on line 3751. If we're in secure mode and we have INET sockets, then sl_recv will be set to NULL, else we will listen. No listening happens in secure mode for INET sockets.

Now I'm even more confused. :) hints.ai_socktype = SOCK_DGRAM in the sole caller of socksetup(), so when is it the case that ai->ai_socktype != SOCK_DGRAM?

Don't worry. I was and still am extremely confused and frustrated with this socket setup code. I don't know why that check is there, but I wanted to avoid meddling with this routine as little as possible so I didn't break anything. If I had the time, I'd re-write the entire thing. :)

This patch does fix the kevent() thrashing. but you're definitely right. It would be best to just not add "safe mode" sockets to kqueue at all. It would be a simple check, if (sl->sl_recv == NULL) then don't add it. I might extend this patch to include that logic.

In D41526#947150, @jfree wrote:

In D41526#947145, @markj wrote:

In D41526#947144, @jfree wrote:

In D41526#947143, @markj wrote:

I don't really understand this change. You wrote, "To fix this, listen() should not be called if syslogd is in secure mode.", but nothing has changed with respect to listen() calls. All that's changed is that we don't call shutdown().

If the problem is that data on unread sockets is thrashing kevent(), why not avoid creating EVFILT_READ events for those sockets?

Notice the else on line 3751. If we're in secure mode and we have INET sockets, then sl_recv will be set to NULL, else we will listen. No listening happens in secure mode for INET sockets.

Now I'm even more confused. :) hints.ai_socktype = SOCK_DGRAM in the sole caller of socksetup(), so when is it the case that ai->ai_socktype != SOCK_DGRAM?

Don't worry. I was and still am extremely confused and frustrated with this socket setup code. I don't know why that check is there, but I wanted to avoid meddling with this routine as little as possible so I didn't break anything. If I had the time, I'd re-write the entire thing. :)

This patch does fix the kevent() thrashing. but you're definitely right. It would be best to just not add "safe mode" sockets to kqueue at all. It would be a simple check, if (sl->sl_recv == NULL) then don't add it. I might extend this patch to include that logic.

I believe that you could leave the block which calls listen() alone (and I do think that block is totally bogus and could be deleted, syslogd doesn't handle listening sockets) and the problem would still be gone. Is that right?

I believe that you could leave the block which calls listen() alone (and I do think that block is totally bogus and could be deleted, syslogd doesn't handle listening sockets) and the problem would still be gone. Is that right?

No. If we listen() and sl_recv == NULL, then EVFILT_READ will listen for incoming connections, keep receiving them, but never read them. They will stay on the kqueue indefinitely and will thrash it.

This problem can be circumvented by not adding sockets with sl_recv == NULL to the kqueue, but then those sockets are listening for no reason. I figured that we should only listen if out sl_recv != NULL, this way the socket's data can be read upon EVFILT_READ. But like you said, I don't even know if it should be listening in the first place since syslogd doesn't support tcp connections in its current state.