utils: Handle close_range more gracefully#4596
Conversation
d212348 to
0bdf924
Compare
| return nil | ||
| } | ||
|
|
||
| logrus.Debugf("close_range failed, closing range one at a time (error: %v)", err) |
There was a problem hiding this comment.
Not sure I get this. If close range fails and we have detected closeRange is available, what does this fallback add?
The errors it can return don't seem like something using the regular close fallback will fix: https://man7.org/linux/man-pages/man2/close_range.2.html.
Am I missing something? Do you see where this new fallback can help?
There was a problem hiding this comment.
It's more defensive, given the existing defensive attempts to see if close_range is available at all. We probe if we can use it, and thusly if for some reason the probe was wrong, we've got the fallback functionality at hand so seems prudent to use it.
There was a problem hiding this comment.
I agree with @evanphx here -- if we can use a fallback, it makes more sense to use it rather than give up and fail right away. Yes, we might still fail later, but oh well, we tried.
There was a problem hiding this comment.
Cool! Should be all ready now.
There was a problem hiding this comment.
While the fallback makes sense, the call to CloseRange is guarded by haveCloseRangeCloexec, meaning if the former fails, the latter is not an adequate check.
Or is it failing only because gvisor doesn't like MaxUint? Maybe it makes sense to add const closeRangeMaxFD and use it in both CloseExecFrom and haveCloseRangeCloexec?
Also, the first and last arguments of close_range are of unsigned int type in both glibc and the kernel. From a cursory look at the kernel code I don't see how specifying MaxUint64 can break things in the kernel.
My bad, I forgot that C int is 32-bit even on a 64-bit platform. |
|
@kolyshkin how are these PRs related to gvisor? I thought gvisor was used as an OCI runtime, as a runc replacement. But from your comment, it seems that is not what is happening here, is it? How is that runc and gvisor are run together in this scenario? |
|
This was running runc within a container managed by runsc, that’s why I’ve
brought it up here.
…On Wed, Jan 22, 2025 at 12:55 PM Rodrigo Campos ***@***.***> wrote:
@kolyshkin <https://github.com/kolyshkin> how are these PRs related to
gvisor? I thought gvisor was used as an OCI runtime, as a runc replacement.
But from your comment, it seems that is not what is happening here, is it?
How is that runc and gvisor are run together in this scenario?
—
Reply to this email directly, view it on GitHub
<#4596 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAAB7PCJDVIJQ3EGW6DEL2MAATPAVCNFSM6AAAAABVPNYGF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBYGI2DGNJWGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Signed-off-by: Evan Phoenix <evan@phx.io>
Signed-off-by: Evan Phoenix <evan@phx.io>
0bdf924 to
33315a0
Compare
|
Happy to do either, no rush.
…On Thu, Feb 6, 2025 at 7:08 PM Kir Kolyshkin ***@***.***> wrote:
@cyphar <https://github.com/cyphar> PTAL
@evanphx <https://github.com/evanphx> is this something that you'd like
to be backported to runc v1.2.x, or is it OK for you to wait for runc v1.3?
—
Reply to this email directly, view it on GitHub
<#4596 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAAB2EZ2PU55RQTPJ27GT2OQPUTAVCNFSM6AAAAABVPNYGF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBRHAZDQNRXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
This fixes 2 things: