Comments concerning Enhanced Flash Patch

[John Rose]

Hi Mark-

> o In rtas_do_extended_delay(), should the blocking state of the current
>   task be TASK_UNINTERRUPTIBLE?  If you're waiting on hardware, you
>   normally want to block uninterruptibly however this doesn't appear
>   to be the case here.  Can someone please comment on this?

To catch up the rest of the list, we have changed the handling code for
the "extended RTAS delay" code to use schedule_timeout() rather than
udelay().  The extended delay time can be up to 100 secs, which is
excessive for udelay().

As far as this question, I think you should be able to signal a task
while it's blocking for one of these delays.  However, my initial patch
didn't check for pending signals after schedule_timeout() returns.
Mark, I'll have a patch out to you shortly that does.

> o Although there is enforcement for a single opener, there is concern
>   about providing mutual exclusion for multiple writers.  Creating new
>   processes/threads via fork/pthread_create in conjunction with possible
>   effects from dup(2) could introduce the risk of data corruption.  Red
>   Hat suggests the use of a semaphore that guards reads and writes either
>   in the entire module or for a specific file.

Accounting for possible/reasonable user error vs. keeping the code
simple.  In the realm of /proc/ppc64 files, there are _plenty_ of places
where this type of data corruption could occur.  Errinjct and scanlog
are two quick examples.

My additions to the flash code did not introduce this vulnerability, but
regardless, it doesn't seem probable to me.  A user who writes a
pthreaded app for such a job is just begging for trouble :)  If I'm
wrong, we should address this problem across the board.

> o In manage_flash() and validate_flash(), there was concern about the
>   worst case elapsed length of time for the execution of the loop.
>   Terminating the loop after some threshold and returning an error code
>   (-EIO?)  is one suggested solution.  Please advise whether this is an
>   acceptable solution or whether a problem even exists at all.

I don't think that any of the RTAS-related kernel code does this.  The
case of firmware endlessly returning busy seems remote to me.
Opinions?  Another problem that exists across the board, if it's a real
possibility.

> o In rtas_flash_init(), there is the possibility for memory leaks in the
>   failure cases.  In addition, there is also the possibility of
>   dereferencing a null pointer in initialize_flash_pde_data() via dp if
>   create_flash_pde() returns null.  This really must be fixed.

Agreed, will be in patch.

Thanks-
John


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/