linux-kernel - Re: [PATCH v2] staging: tidspbridge: protect dmm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTi=xmxkWnhuYcjtDNDHgep_2EztfGXDv+winW2Y5@mail.gmail.com>
Date:	Tue, 28 Dec 2010 12:56:46 +0200
From:	Ohad Ben-Cohen <ohad@...ery.com>
To:	Felipe Contreras <felipe.contreras@...il.com>
Cc:	Felipe Contreras <felipe.contreras@...ia.com>,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	greg@...ah.com, omar.ramirez@...com, fernando.lugo@...com,
	nm@...com, ameya.palande@...ia.com, h-kanigeri2@...com
Subject: Re: [PATCH v2] staging: tidspbridge: protect dmm_map properly

On Tue, Dec 28, 2010 at 12:36 PM, Felipe Contreras
<felipe.contreras@...il.com> wrote:
>> 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.

Right. and both patches do that. one adds locks, and one doesn't.

> However, as I sad, everybody is using proc_map() and proc_un_map()
> which take a lock, and there are no complaints.

I didn't complain about that; I didn't like you adding locks to the DMA API.

> Ok, can I get your Ack?

Frankly, I don't like the locks you are adding. But as I said, I
wouldn't resist them as long as it's temporary.

> 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.

I still don't know how exactly you triggered the bug: is gst-dsp
multithreaded ? and one of its threads invoked proc_un_map() while
another thread called proc_begin_dma() ?

Anyhow, a thread that is calling proc_*_dma() will both increase the
reference count and decrease it back before going back to user space.
Otherwise your patch would be problematic as well - who will unlock
the mutex you take in proc_*_dma() ?

>>> 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.

Right. But we have a fix that doesn't add any additional locking... I
don't see why it can't be taken now, but as I said, I wouldn't resist
staging it for the next cycle.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/