mbox series

[00/10] floppy driver cleanups (deobfuscation)

Message ID 20200224212352.8640-1-w@1wt.eu (mailing list archive)
Headers show
Series floppy driver cleanups (deobfuscation) | expand

Message

Willy Tarreau Feb. 24, 2020, 9:23 p.m. UTC
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(-)

Comments

Denis Efremov (Oracle) Feb. 26, 2020, 2:57 p.m. UTC | #1
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
Linus Torvalds Feb. 26, 2020, 5:49 p.m. UTC | #2
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
Willy Tarreau Feb. 26, 2020, 6:41 p.m. UTC | #3
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
Willy Tarreau Feb. 29, 2020, 2:13 p.m. UTC | #4
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
Linus Torvalds Feb. 29, 2020, 3:58 p.m. UTC | #5
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
Ondrej Zary Feb. 29, 2020, 11:19 p.m. UTC | #6
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.
Willy Tarreau March 1, 2020, 6:46 a.m. UTC | #7
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
Ondrej Zary March 1, 2020, 5:01 p.m. UTC | #8
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.