Re: [Mjpeg-developer] [PATCH] zr36067: Debugging cleanups (updated again)

[Trent P. <xy...@sp...>]

On Sat, 9 Jun 2007, Jean Delvare wrote:
> On Sat, 26 May 2007 07:17:44 -0700 (PDT), Trent Piepho wrote:
> >
> > That doesn't work, jpg_bufsize is used at other times too, such as in a
call
> > to REQBUFS.  I think it's a general assumption that people setting parameters
> > via sysfs should know what they are doing.  There must be thousands of module
> > parameter one can set to values that aren't correct via sysfs.  It's easiest
> > and most efficient to check a module's parameters in its init function.
>
> Easiest, most efficient and partly incorrect IMHO. My opinion is that
> one shouldn't make a module parameter writable through sysfs without
> also making sure that the code will behave properly when values are
> actually written to the sysfs files. But you seem to have a different
> opinion on this matter, and given that I am neither the zr36067 driver
> maintainer not a v4l subsystem maintainer, my opinion probably doesn't
> matter that much.

In my option, setting parameters via sysfs is pretty much a hack.  I don't
think there is a single settable parameter in the entire kernel that is
100% safe.

> > Sure it can.  You must understand, lacking a memory barrier the C compiler
> > can and will reorder your code.  If reading or writing an integer variable
> > was atomic, and if comparisons were done in the order written, and if each
> > access to a variable caused it to be read from memory, then your code would
> > fix a race condition.  But none of those things are true, and so your code
> > doesn't actually fix a race condition.
>
> Sorry but I have to insist here, because you still seem to miss the key
> problem I have been pointing out.

I understand the problem you've pointed out.  What I'm saying is that your
patch doesn't fix it.  It is _not possible_ to fix it without proper
locking.  You can re-order the if statements all you want, it's not going
to fix a race condition any more than changing whitespace will.

> Here we have a variable which can have 3 different states A, B and C. I
> _do_ agree that, if the variable is originally in state A, and the user
> changes it to state B, and for a short period of time, the driver still
> considers it is in state A, this is a only a minor race condition,
> which may not be worth addressing.

It's more complex than that.  There is no assurance that an integer
variable will be updated atomically.  It is entirely possible to change an
integer from -1 to 0, and have another CPU see a value which is neither -1
nor 0, but something like 255.

> But the code after your patch does something different. It is such
> that, when the variable is in state A, and the user changes it to state
> B, the driver might, for a short period of time, do something which
> should only occur when the variable is in state C. This is a major race

If you look at the generated code, there is in fact no race condition at
all on ia32:

        movl    lock_norm, %ecx     # lock_norm, lock_norm.330
        testl   %ecx, %ecx          # lock_norm.330
        je      .L373               #,
        movl    1348(%ebx), %eax    # <variable>.norm, <variable>.norm
        cmpl    %edx, %eax          # norm, <variable>.norm
        je      .L373               #,
.LVL336:
        decl    %ecx                # lock_norm.330
        jg      .L410               #, (L410 is lock_norm>1 block)
	(the lock_norm>1 else block goes here)

As you can see, the value of lock_norm is only read from memory a single
time for the entire lock_norm block of code.  Since there is no lock prefix
on the movl instruction, it's not atomic on an SMP system, but other than
that there is no possibility of a race condition.

In effect, this C code:

if (lock_norm && norm != zr->norm) {
	if (lock_norm > 1) {
		/* stuff */
	} else {
		/* other stuff */
	}
}

Is compiled into this:

tmpc = lock_norm;
if (tmpc) {
        tmpa = zr->norm;
	if (norm != tmpa) {
		if (--tmpc > 0) {
			/* stuff */
		} else {
			/* other stuff */
		}
	}
}

In this case lock_norm can change all you want during this block of code
and it doesn't make any difference, since it always use the copy it made in
tmpc.

> condition. This is a _bug_, nothing less. And given how easy it is to
> change the code to fall under the "minor race condition" case above, I
> believe this should be done.

But it's not easy to fix!  Currently the C code compiles to something that
doesn't have a race condition.  For all we know, your modification will
compile to something that _does_ have a race condition!  Unless you
actually use atomic types, volatile variables, memory barriers, and/or
spinlocks, it doesn't matter what C code you write, it won't prevent
a possible race.