Re: [PATCH v2] staging: tidspbridge: protect dmm_map properly

From: Felipe Contreras
Date: Tue Dec 28 2010 - 05:37:05 EST


On Tue, Dec 28, 2010 at 8:33 AM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote:
> On Mon, Dec 27, 2010 at 3:55 PM, Felipe Contreras
> <felipe.contreras@xxxxxxxxx> wrote:
>> So, effectively, serializing the proc_begin_dma() and proc_end_dma()
>> would not affect anyone negatively for the time being.
>
> You can never really tell who is using the kernel (or will be using
> this kernel version), how and under which workload.

No, but it's better to address real issues rather than hypothetical.

However, as I sad, everybody is using proc_map() and proc_un_map()
which take a lock, and there are no complaints. This patch would make
proc_begin_dma() and proc_end_dma() as slow as the map operations, so
even these hypothetical users would not get affected negatively.

>> For the long-term goal I agree with that approach, however, first, I
>> think my patch should be applied, since it's fixing a problem using an
>> already existing and actively excersised mechanism. In fact, I think
>> this should be pushed to 2.6.37 as:
>>
>> Â1) prevents a kernel panic
>> Â2) the issue is reproducible and clearly identified
>> Â3) the patch is small and obvious
>
> Both patches are (IMHO). But frankly I don't mind your patch will be
> applied now as long as it doesn't stay. I can rebase my patch against
> it after it is applied, and send separately.

Ok, can I get your Ack? I guess Omar would be able to push it to Greg,
and perhaps it would make .37.

>> This approach looks cleaner, however, we need a flag in
>> remove_mapping_information() in order to force the removal, otherwise
>> there will be memory leaks. Imagine a process crashes, and
>> remove_mapping_information() returns -EBUSY.
>
> Can't happen; both proc_*_dma() operations decrease the reference
> count before exiting, so it's not up to the application.

Then why did you add that check for is_map_obj_used(), and then return
-EBUSY? If that can happen, then it can happen when the application is
crashing; user-space crashes while kernel-space is in the middle of a
proc_*_dma() operation.

>> Sure, but I see this as a broader effort to have finer locking, part of
>> this should be to remove the already existing proc_lock.
>
> Having bad locking is not an excuse for adding more.

No, but not being a permanent solution is not an excuse for not fixing
a kernel panic right away.

> Anyway, as I said, I don't really mind your patch will be applied as
> long as it is a temporary workaround.

Can you Ack?

--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/