Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ppoll eintr #25411

Closed
wants to merge 2 commits into from
Closed

Fix ppoll eintr #25411

wants to merge 2 commits into from

Conversation

fbuihuu
Copy link
Contributor

@fbuihuu fbuihuu commented Nov 16, 2022

No description provided.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Nov 16, 2022

This complements #21564.

@poettering
Copy link
Member

hmm, I think it would be simpler to just add the if (ERRNO_IS_TRANSIENT(r)) continue; to each ppoll() and fd_wait_for_event() call site where signals are to be expected. I mean, some already have that, maybe just add that for the remaining ones where we want that?

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks util-lib labels Nov 16, 2022
@fbuihuu
Copy link
Contributor Author

fbuihuu commented Nov 17, 2022 •
edited
Loading

hmm, I think it would be simpler to just add the if (ERRNO_IS_TRANSIENT(r)) continue; to each ppoll() and fd_wait_for_event() call site where signals are to be expected. I mean, some already have that, maybe just add that for the remaining ones where we want that?

I agree that some of the fd_wait_for_event() call sites also needs to be fixed.

However a call to the new helper is self documented and clearly indicates the fact that signals are expected and the interrupted syscall should be retried. I'm not sure to see how each ppoll() call site dealing itself with the automatic retry and with the logic to update the timeout value could be simpler.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Nov 17, 2022

Thinking more about this I think "sd-bus: fix bus_poll() to handle EINTR properly" may be the wrong approach. Indeed we're dealing with library code here and we have no idea how the application linked against the lib handles signals (it might have installed signal handlers and want us to return immediately if we got interrupted for example).

Hence automatically retrying syscalls after being interrupted by a signal or hiding the fact that we got interrupted in the middle of the process of bus_call_method() like b3d06b9 did seems incorrect.

WDYT ?

@fbuihuu
io-util: introduce safe_ppoll_usec() helper
5290eae 
Most of ppoll(2) users should (and already do) handle EINTR properly in case
the system call receives a signal before any requested event occurs.

Hence let's introduce a helper that just does that automatically and use the
new helper where it makes sense.

No functional change.
@fbuihuu
sd-bus: fix bus_poll() to handle EINTR properly
56ddbf2 
bus_ppoll() relies on ppoll(2) which can return EINTR when interrupted by a
signal. bus_ppoll() should be prepared to handle this case properly by
restarting the system call.

Hence let's make it call safe_ppoll_usec() instead since it does that
automatically for us.

This should address one of the cases that lead to the following error, which
might occur when shutting down the system:

 login[1489]: pam_systemd(login:session): Failed to release session: Interrupted system call

IOW this patch closes another possible root cause of issue systemd#18817.
@fbuihuu
Copy link
Contributor Author

fbuihuu commented Nov 21, 2022 •
edited
Loading

So I pushed mostly the same version but with the fix addressing Yu's issue.

I'm still not convinced that we should add if (ERRNO_IS_TRANSIENT(r)) continue; instead like @poettering suggested. Also it might be better to just restart ppoll(2) syscall rather than retrying the whole loop. It might be the wrong thing to do in some cases (imagine a loop that consists in write(fd, ...); ppoll(fd,...); read(fd, ...), here we wouldn't want to write the data again) and we shouldretry the syscall that returns EINTR after all.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Nov 21, 2022

Regarding fd_wait_for_event() it's a bit trickier as it's not obvious for some call sites to see whether we should ignore signals or not so I prefer leaving this for someone else ;)

@poettering poettering mentioned this pull request Nov 22, 2022
Merged
@poettering
Copy link
Member

→ #25483

@yuwata yuwata closed this Nov 24, 2022
@fbuihuu fbuihuu deleted the fix-ppoll-eintr branch November 28, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuwata yuwata yuwata left review comments

Assignees
No one assigned
Labels
needs-discussion 🤔 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks util-lib
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fbuihuu @poettering @yuwata