On Mon, May 11, 2009 at 6:47 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote: > Hi Hari, > > From: "ext Kanigeri, Hari" <h-kanigeri2@xxxxxx> > Subject: RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size > Date: Mon, 11 May 2009 17:09:03 +0200 > >> Hi Doyu-san, >> >> > A buffer shared with MPU and DSP has to be aligned on both cache line >> > size to avoid memory corrupton with some DSP cache operations. Since >> > there's no way for dspbridge to know how the shared buffer will be >> > used like: "read-only", "write-only", "rw" through its life span, any >> > shared buffer passed to DSP should be on this alignment. This patch >> > adds checking those shared buffer alignement in bridgedriver cache >> > operations and prevents userland applications from causing the above >> > memory corruption. >> > >> >> -- It looks like the buffer that are passed to the Bridge are not necessarily 128 byte aligned. >> >> This is the comment I received from Nikhil Mande (MM Engineer). >> >> [Nikhil Mande] "They are not necessarily "aligned" all the time. >> Sometimes they just have 128 byte padding at the start & end of the buffer and the buffer pointer passed to bridge is not guaranteed to be 128 aligned. >> So if you are adding such a check, it will require modifications OMX >> components & test apps." > > The above means that passing unaligned address to dsp can cause memory > corruption in kernel and this problem can be avoided only in the case > where OMX(userland) is always supposed to pass aligned address. The > above assumption cannot be accepted because kernel should always be > robust against any incorrect behaviour of userland application. For > example, even if an userland application passes any incorrect > parameters to kernel through ioctl(), kernel shouldn't crash, but just > returns -EINVAL. Yes, but flush and invalidate are not the right places to do that. An application in user-space might be buggy and decide not to do any flush or invalidation, therefore the checks won't happen. A more proper use-case could be to flush only parts of a big buffer. Also, you are not checking for the upper limit, which also should be 128-byte aligned. A more correct place to do this would be on PROC_Map, which user-space cannot avoid in any way, and also you have the upper limit too. Unfortunately TI's OpenMAX IL is sending a fake upper limit (page aligned). IMHO the only sensible way to approach this is to create a new ioctl for a special restrictive mapping where user-space specifies if the memory area is read-only or write-only and the size must be the real size. This also has advantage that it does not break legacy applications. While we are at it, this new mapping could do PROC_ReserveMemory at the same time so that the user-space application doesn't get a chance to modify the pa. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
