| Message ID | 20200224212352.8640-1-w@1wt.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | floppy driver cleanups (deobfuscation) | expand |
On 2/25/20 12:23 AM, Willy Tarreau wrote: > As indicated in commit 2e90ca6 ("floppy: check FDC index for errors > before assigning it") there are some surprising effects in the floppy > driver due to some macros referencing global or local variables while > at first glance being inoffensive. > > This patchset aims at removing these macros and replacing all of their > occurrences by the equivalent code. Most of the work was done under > Coccinelle's assistance, and it was verified that the resulting binary > code is exactly the same as the original one. > > The aim is not to make the driver prettier, as Linus mentioned it's > already not pretty. It only aims at making potential bugs more visible, > given almost all latest changes to this driver were fixes for out-of- > bounds and similar bugs. > > As a side effect, some lines got longer, causing checkpatch to complain > a bit, but I preferred to let it complain as I didn't want to break them > apart as I'm already seeing the trap of going too far here. > > The patches are broken by macro (or sets of macros when relevant) so > that each of them remains reviewable. > > I can possibly go a bit further in the cleanup but I haven't used > floppies for a few years now and am not interested in doing too much > on this driver by lack of use cases. For patches 1-10. [x] eye checked the changes [x] bloat-o-meter and .s diff show no real changes [x] tested that kernel builds after every patch [x] floppy targeted fuzzing with kasan+ubsan reveals no *new* issues (required mainly to test the previous patch) If Linus has no objections (regarding his review) I would prefer to accept 1-10 patches rather to resend them again. They seems complete to me as the first step. I've placed the patches here: https://github.com/evdenis/linux-floppy/commits/floppy-next Thanks, Denis
On Wed, Feb 26, 2020 at 6:57 AM Denis Efremov <efremov@linux.com> wrote: > > If Linus has no objections (regarding his review) I would prefer to > accept 1-10 patches rather to resend them again. They seems complete > to me as the first step. I have no objections, and the patches 11-16 seem to have addressed all my "I wish.." concerns too (except for the "we should rewrite or sunset the driver entirely"). Looks fine to me. Linus
On Wed, Feb 26, 2020 at 09:49:05AM -0800, Linus Torvalds wrote: > On Wed, Feb 26, 2020 at 6:57 AM Denis Efremov <efremov@linux.com> wrote: > > > > If Linus has no objections (regarding his review) I would prefer to > > accept 1-10 patches rather to resend them again. They seems complete > > to me as the first step. > > I have no objections, and the patches 11-16 seem to have addressed all > my "I wish.." concerns too (except for the "we should rewrite or > sunset the driver entirely"). Sorry if I broke your dream :-) > Looks fine to me. Thanks! Willy
Hi Linus, On Wed, Feb 26, 2020 at 09:49:05AM -0800, Linus Torvalds wrote: > On Wed, Feb 26, 2020 at 6:57 AM Denis Efremov <efremov@linux.com> wrote: > > > > If Linus has no objections (regarding his review) I would prefer to > > accept 1-10 patches rather to resend them again. They seems complete > > to me as the first step. > > I have no objections, and the patches 11-16 seem to have addressed all > my "I wish.." concerns too (except for the "we should rewrite or > sunset the driver entirely"). Looks fine to me. So I continued to work on this cleanup and regardless of the side I attacked this hill, it progressively stalled once I got closer to the interrupt and delayed work stuff. I understood the root cause of the problem, it's actually double: - single interrupt for multiple FDCs : this means we need to have some global context to know what we're working with. It is not completely true because we could very well have a list of pending operations per FDC to scan on interrupt and update them when the interrupt strikes and the FDC reports a completion. I noticed that raw_cmd, raw_buffer, inr, current_req, _floppy, plus a number of function pointers used to handle delayed work should in fact be per-FDC if we didn't want to change them at run time ; - single DMA channel for multiple FDCs : regardless of what could be done for the interrupt above, we're still stuck with a single DMA setup at once, which requires to issue reset_fdc() here and there, making it clear we can't work with multiple FDCs in parallel. Given the number of places where such global variables are set, I'm not totally confident in the fact that nowhere we keep a setting that was assigned for the FDC in the previous request. For example a spurious (or late) interrupt could slightly affect the fdc_state[] and maybe even emit FD_SENSEI to the current controller. Also this comment in floppy_interrupt() made me realize the driver pre-dates SMP support: /* It is OK to emit floppy commands because we are in an interrupt * handler here, and thus we have to fear no interference of other * activity. */ It seems that changing the current FDC is still only protected by the fdc_busy bit, but that the interrupt handler doesn't check if anything is expected before touching bits related to current_fdc. All this made me wonder if we really want to continue to maintain the support for multiple FDCs. I checked all archs, and only x86, alpha and powerpc support more than one FDC, 2 to be precise (hence up to 8 drives). I have the impression that if we keep a single FDC we'll have a cleaner code that doesn't need to change global settings under us when doing resets or scans. So I don't know if that's something interesting to pursue. I also noticed that a lot of global variables are inter-dependent and should probably be moved to fdc_state[] so that what's per-FDC is now more explicit, except that this struct is exposed to userland via the FDGETFDCSTAT ioctl (but that could be changed so that the fdc_state is just a struct inside a per-fdc larger struct). At least now I get a better picture of the little ROI felt trying to clean this, and I don't think that even a complete rewrite as you suggested would address all the issues at all. So if you or Denis think there's some value in me continuing to explore one of these areas, I can continue, otherwise I can simply resend the last part of my series with the few missing Cc and be done with it. Willy
On Sat, Feb 29, 2020 at 8:14 AM Willy Tarreau <w@1wt.eu> wrote: > > So if you or Denis think there's some value in me continuing to explore > one of these areas, I can continue, otherwise I can simply resend the > last part of my series with the few missing Cc and be done with it. It's fine - this driver isn't worth spending a ton of effort on. The only users are virtualization, and even they are going away because floppies are so small, and other things have become more standard anyway (ie USB disk) or easier to emulate (NVMe or whatever). So I suspect the only reason floppy is used even in that area is just legacy "we haven't bothered updating to anything better and we have old scripts and images that work". Linus
On Saturday 29 February 2020 16:58:11 Linus Torvalds wrote: > On Sat, Feb 29, 2020 at 8:14 AM Willy Tarreau <w@1wt.eu> wrote: > > > > So if you or Denis think there's some value in me continuing to explore > > one of these areas, I can continue, otherwise I can simply resend the > > last part of my series with the few missing Cc and be done with it. > > It's fine - this driver isn't worth spending a ton of effort on. > > The only users are virtualization, and even they are going away > because floppies are so small, and other things have become more > standard anyway (ie USB disk) or easier to emulate (NVMe or whatever). > > So I suspect the only reason floppy is used even in that area is just > legacy "we haven't bothered updating to anything better and we have > old scripts and images that work". > > Linus > There are real users with real floppy drives out there.
On Sun, Mar 01, 2020 at 12:19:14AM +0100, Ondrej Zary wrote: > On Saturday 29 February 2020 16:58:11 Linus Torvalds wrote: > > On Sat, Feb 29, 2020 at 8:14 AM Willy Tarreau <w@1wt.eu> wrote: > > > > > > So if you or Denis think there's some value in me continuing to explore > > > one of these areas, I can continue, otherwise I can simply resend the > > > last part of my series with the few missing Cc and be done with it. > > > > It's fine - this driver isn't worth spending a ton of effort on. > > > > The only users are virtualization, and even they are going away > > because floppies are so small, and other things have become more > > standard anyway (ie USB disk) or easier to emulate (NVMe or whatever). > > > > So I suspect the only reason floppy is used even in that area is just > > legacy "we haven't bothered updating to anything better and we have > > old scripts and images that work". > > > > Linus > > > > There are real users with real floppy drives out there. OK thanks for the feedback. Then I'll continue the minimum cleanups to try to focus on maintainability and on the principle of least surprise, and I'll have a quick look at the possible simplifications brought by the limitation to one FDC, in case that really helps. Willy
On Sunday 01 March 2020 07:46:01 Willy Tarreau wrote: > On Sun, Mar 01, 2020 at 12:19:14AM +0100, Ondrej Zary wrote: > > On Saturday 29 February 2020 16:58:11 Linus Torvalds wrote: > > > On Sat, Feb 29, 2020 at 8:14 AM Willy Tarreau <w@1wt.eu> wrote: > > > > So if you or Denis think there's some value in me continuing to > > > > explore one of these areas, I can continue, otherwise I can simply > > > > resend the last part of my series with the few missing Cc and be done > > > > with it. > > > > > > It's fine - this driver isn't worth spending a ton of effort on. > > > > > > The only users are virtualization, and even they are going away > > > because floppies are so small, and other things have become more > > > standard anyway (ie USB disk) or easier to emulate (NVMe or whatever). > > > > > > So I suspect the only reason floppy is used even in that area is just > > > legacy "we haven't bothered updating to anything better and we have > > > old scripts and images that work". > > > > > > Linus > > > > There are real users with real floppy drives out there. > > OK thanks for the feedback. Then I'll continue the minimum cleanups to > try to focus on maintainability and on the principle of least surprise, > and I'll have a quick look at the possible simplifications brought by > the limitation to one FDC, in case that really helps. Thank you very much for the work. I haven't ever seen a machine with more than single FDC so that case might be hard to test. There are some ISA FDCs with configurable I/O addresses (maybe I have one of them somewhere) but they might not work properly together with on-board super I/O FDCs. The most common case - one FDC with at most two drives should be enough for the modern simplified driver.
As indicated in commit 2e90ca6 ("floppy: check FDC index for errors before assigning it") there are some surprising effects in the floppy driver due to some macros referencing global or local variables while at first glance being inoffensive. This patchset aims at removing these macros and replacing all of their occurrences by the equivalent code. Most of the work was done under Coccinelle's assistance, and it was verified that the resulting binary code is exactly the same as the original one. The aim is not to make the driver prettier, as Linus mentioned it's already not pretty. It only aims at making potential bugs more visible, given almost all latest changes to this driver were fixes for out-of- bounds and similar bugs. As a side effect, some lines got longer, causing checkpatch to complain a bit, but I preferred to let it complain as I didn't want to break them apart as I'm already seeing the trap of going too far here. The patches are broken by macro (or sets of macros when relevant) so that each of them remains reviewable. I can possibly go a bit further in the cleanup but I haven't used floppies for a few years now and am not interested in doing too much on this driver by lack of use cases. Willy Tarreau (10): floppy: cleanup: expand macro FDCS floppy: cleanup: expand macro UFDCS floppy: cleanup: expand macro UDP floppy: cleanup: expand macro UDRS floppy: cleanup: expand macro UDRWE floppy: cleanup: expand macro DP floppy: cleanup: expand macro DRS floppy: cleanup: expand macro DRWE floppy: cleanup: expand the R/W / format command macros floppy: cleanup: expand the reply_buffer macros drivers/block/floppy.c | 971 +++++++++++++++++++++++++------------------------ 1 file changed, 499 insertions(+), 472 deletions(-)