| Message ID | 20230911132551.1421276-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tpm: fix crash when FD >= 1024 | expand |
On 9/11/23 09:25, marcandre.lureau@redhat.com wrote: > From: Marc-Andr Lureau <marcandre.lureau@redhat.com> > > Replace select() with poll() to fix a crash when QEMU has a large number > of FDs. > > Fixes: > https://bugzilla.redhat.com/show_bug.cgi?id=2020133 For backporting I think we should also add this tag here: Fixes: ca64b08638 ("tpm: Move backend code under the 'backends/' directory") Though RETRY_ON_EINTR was only introduced in 8.0.0-rc0. What's the right tag for backporting then? Stefan > Signed-off-by: Marc-Andr Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > backends/tpm/tpm_util.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c > index a6e6d3e72f..1856589c3b 100644 > --- a/backends/tpm/tpm_util.c > +++ b/backends/tpm/tpm_util.c > @@ -112,12 +112,8 @@ static int tpm_util_request(int fd, > void *response, > size_t responselen) > { > - fd_set readfds; > + GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } }; > int n; > - struct timeval tv = { > - .tv_sec = 1, > - .tv_usec = 0, > - }; > > n = write(fd, request, requestlen); > if (n < 0) { > @@ -127,11 +123,8 @@ static int tpm_util_request(int fd, > return -EFAULT; > } > > - FD_ZERO(&readfds); > - FD_SET(fd, &readfds); > - > /* wait for a second */ > - n = select(fd + 1, &readfds, NULL, NULL, &tv); > + n = RETRY_ON_EINTR(g_poll(fds, 1, 1000)); > if (n != 1) { > return -errno; > }
12.09.2023 03:08, Stefan Berger wrote: > > On 9/11/23 09:25, marcandre.lureau@redhat.com wrote: >> From: Marc-Andr Lureau <marcandre.lureau@redhat.com> >> >> Replace select() with poll() to fix a crash when QEMU has a large number >> of FDs. >> >> Fixes: >> https://bugzilla.redhat.com/show_bug.cgi?id=2020133 > > For backporting I think we should also add this tag here: > > Fixes: ca64b08638 ("tpm: Move backend code under the 'backends/' directory") It's nice to have Fixes tags generally. Yes, it helps backporting a little bit, but it is mostly about choosing which changes might be appropriate when there's no to-stable/to-backport markers/tags whatsoever. If you already know for sure some change should be picked up for stable, it's better to add Cc: qemu-stable@. With Fixes also in place, besides its usefulness for other purposes, it helps me to see which older versions needs this, but usually it's relatively easy to determine even without Fixes: tag. Many changes picked up for stable do not have such tag just because there's no single commit which introduced an issue, or some other situation. > > Though RETRY_ON_EINTR was only introduced in 8.0.0-rc0. What's the right tag for backporting then? There's no such tag. If you know already there's possible issue with older versions (and this is exactly the case), any comment about this might help for sure. This your note saved me a compile (which would fail for sure), after which I would find commit v7.2.0-538-g8b6aa69365 Author: Nikita Ivanov <nivanov@cloudlinux.com> Date: Sun Oct 23 12:04:21 2022 +0300 Refactoring: refactor TFR() macro to RETRY_ON_EINTR() the same way I did now. If you're trying to find a way to make this new fix be "more backportable", maybe by avoiding using a feature designed especially for this, - I think this is not productive, the priority is definitely to have better "master", and think about what to do with earlier versions only after that. In this case, and in about 5 other examples from today, the thought about stable releases best be done when introducing wide changes, like this commit above which replaced TFR with RETRY_ON_EINTR. Since this new macro will be used everywhere for sure, the best way would be to split that single patch into 3: first one introducing the new RETRY_ON_EINTR(), second converting all users of TFR to RETRY_ON_EINTR, and 3rd (which can be folded into second) removing TFR which is now unused. This way I can cherry-pick just the first patch easily if needed. But once again, the priority should be master, not backports. Thanks, /mjt
12.09.2023 03:08, Stefan Berger пишет: > > On 9/11/23 09:25, marcandre.lureau@redhat.com wrote: >> From: Marc-Andr Lureau <marcandre.lureau@redhat.com> >> >> Replace select() with poll() to fix a crash when QEMU has a large number >> of FDs. >> >> Fixes: >> https://bugzilla.redhat.com/show_bug.cgi?id=2020133 > > Fixes: ca64b08638 ("tpm: Move backend code under the 'backends/' directory") Heh. I noticed this only now. No, this is not the commit which introduced the breakage. It is either commit 56a3c24ffc11955ddc7bb21362ca8069a3fc8c55 Author: Stefan Berger <stefanb@linux.vnet.ibm.com> Date: Tue May 26 16:51:06 2015 -0400 tpm: Probe for connected TPM 1.2 or TPM 2 which introduced select() in the first place (provided similar select() hasn't been used in there before. Or some other commit somewhere else which allowed to have large number of filedescriptors - provided it wasn't possible before. But definitely not a commit which just moved file into another subdir :) /mjt
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c index a6e6d3e72f..1856589c3b 100644 --- a/backends/tpm/tpm_util.c +++ b/backends/tpm/tpm_util.c @@ -112,12 +112,8 @@ static int tpm_util_request(int fd, void *response, size_t responselen) { - fd_set readfds; + GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } }; int n; - struct timeval tv = { - .tv_sec = 1, - .tv_usec = 0, - }; n = write(fd, request, requestlen); if (n < 0) { @@ -127,11 +123,8 @@ static int tpm_util_request(int fd, return -EFAULT; } - FD_ZERO(&readfds); - FD_SET(fd, &readfds); - /* wait for a second */ - n = select(fd + 1, &readfds, NULL, NULL, &tv); + n = RETRY_ON_EINTR(g_poll(fds, 1, 1000)); if (n != 1) { return -errno; }