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.

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

preload: Only emulate ioctls on emulated devices #264

Merged
merged 3 commits into from
Jan 2, 2025
Merged

preload: Only emulate ioctls on emulated devices #264

merged 3 commits into from
Jan 2, 2025

Conversation

martinpitt
Copy link
Owner

Don't invoke the remote ioctl handler for host devices. This fixes e.g.
ioctls on /dev/tty or other standard devices which the tested program
may want to do.

This bug was hidden by the conditional check in t_usbfs_ioctl_static()
which skipped the check if /dev/tty wasn't accessible -- which is the
case in meson test. Move to a FIONREAD ioctl on /dev/stdout, which
works on ttys and pipes, i.e. everywhere. Make that check unconditional.

Don't clobber errno with the failures of checking for the emulation
socket.
Don't invoke the remote ioctl handler for host devices. This fixes e.g.
ioctls on /dev/tty or other standard devices which the tested program
may want to do.

This bug was hidden by the conditional check in t_usbfs_ioctl_static()
which skipped the check if /dev/tty wasn't accessible -- which is the
case in `meson test`. Move to a FIONREAD ioctl on /dev/stdout, which
works on ttys and pipes, i.e. everywhere. Make that check unconditional.
@martinpitt
Copy link
Owner Author

There is still a valgrind issue here, but @benzea I'd appreciate a quick look if that approach works looks sane to you? Do you have a use case for ioctl emulation on real host devices?

@benzea
Copy link
Collaborator

benzea commented Jan 2, 2025

There is still a valgrind issue here, but @benzea I'd appreciate a quick look if that approach works looks sane to you? Do you have a use case for ioctl emulation on real host devices?

No? From what I can tell, that would not make any sense whatsoever.

As for valgrind, that looks like a two-fold issue. i.e. we are reaching out to the daemon for a read() that it will not handle. So it seems that the new logic may not be catching some corner case.

The reason the valgrind error happens is that the ioctl code reads the client memory unconditionally. Even for a read() call, as that is simpler overall.

I think we should wrap the _send for IOCTL_RES_READ_MEM into a block of VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE/VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE to silence the warning. We have had reports of this being annoying in the past and I had not realised that valgrind supports these kind of requests.

EDIT: Inserted the short explanation about why/when this valgrind error happens.

@martinpitt
Copy link
Owner Author

Thanks @benzea ! I found these macros in https://github.com/sstsimulator/sst-valgrind-tracer/blob/master/memcheck/memcheck.h but they are not present in my build root. So I did that with a valgrind suppression instead. I also confirmed that without the suppression, the error also goes away when I add a memset(&ev, 0, sizeof(ev)); to the test (I don't want to commit that though, a suppression is IMHO more correct).

@martinpitt martinpitt marked this pull request as ready for review January 2, 2025 13:29
@benzea
Copy link
Collaborator

benzea commented Jan 2, 2025

Thanks @benzea ! I found these macros in https://github.com/sstsimulator/sst-valgrind-tracer/blob/master/memcheck/memcheck.h but they are not present in my build root. So I did that with a valgrind suppression instead. I also confirmed that without the suppression, the error also goes away when I add a memset(&ev, 0, sizeof(ev)); to the test (I don't want to commit that though, a suppression is IMHO more correct).

Ah, darn, I guess that API is pretty new then.

One could probably do something like getting the vbits, then marking the memory as accessible and setting the vbits again afterwards. As a further step one could even pass the vbits over the wire both ways, enabling the emulation code to read/set the validity bits.

@martinpitt martinpitt merged commit baa934f into main Jan 2, 2025
36 checks passed
@martinpitt martinpitt deleted the ioctl branch January 2, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants