ALSA: Soc_compress open file node allows more than once

ALSA: Soc_compress open file node allows more than once

[b_lkasam]

Hi ALSA team,
This is regarding compress offload framework file operations wrt open 
system call.

For open call to this driver currently allows for same node more than 
once.

This may result in memleak issues or data corruption if same node in use 
by multiple instances.

can we have restriction to avoid open of same node using some refcount?

we can either have refcount under compr->private_data and cross check in 
below function before proceeding.
static int snd_compr_open(struct inode *inode, struct file *f)

Please suggest accordingly.

Thanks
Laxminath Kasam,
Qualcomm.

Re: ALSA: Soc_compress open file node allows more than once

[Takashi Iwai]

On Thu, 23 Nov 2017 14:18:30 +0100,
b_lkasam@codeaurora.org wrote:
> 
> Hi ALSA team,
> This is regarding compress offload framework file operations wrt open
> system call.
> 
> For open call to this driver currently allows for same node more than
> once.
> 
> This may result in memleak issues or data corruption if same node in
> use by multiple instances.

This only depends on the driver side implementation.  There is no
restriction in the core side, as the multiple open is possible
depending on the hardware design and the driver implementation.

> can we have restriction to avoid open of same node using some refcount?
> 
> we can either have refcount under compr->private_data and cross check
> in below function before proceeding.
> static int snd_compr_open(struct inode *inode, struct file *f)
> 
> Please suggest accordingly.

I'd suggest just to add an exclusive open check in the driver side.
The open/close callbacks are protected via a mutex, so it can be a
simple bool flag check.

If the exclusive access is regarded as a common pattern, we can add
some flag to snd_compr later and check it in the core, instead, too.


thanks,

Takashi

Re: ALSA: Soc_compress open file node allows more than once

[Vinod Koul]

On Thu, Nov 23, 2017 at 02:41:00PM +0100, Takashi Iwai wrote:
> On Thu, 23 Nov 2017 14:18:30 +0100,
> b_lkasam@codeaurora.org wrote:
> > 
> > Hi ALSA team,
> > This is regarding compress offload framework file operations wrt open
> > system call.
> > 
> > For open call to this driver currently allows for same node more than
> > once.
> > 
> > This may result in memleak issues or data corruption if same node in
> > use by multiple instances.
> 
> This only depends on the driver side implementation.  There is no
> restriction in the core side, as the multiple open is possible
> depending on the hardware design and the driver implementation.

sounds right to me, also is the leak on core side or your driver?

> 
> > can we have restriction to avoid open of same node using some refcount?
> > 
> > we can either have refcount under compr->private_data and cross check
> > in below function before proceeding.
> > static int snd_compr_open(struct inode *inode, struct file *f)
> > 
> > Please suggest accordingly.
> 
> I'd suggest just to add an exclusive open check in the driver side.
> The open/close callbacks are protected via a mutex, so it can be a
> simple bool flag check.
> 
> If the exclusive access is regarded as a common pattern, we can add
> some flag to snd_compr later and check it in the core, instead, too.

-- 
~Vinod

Re: ALSA: Soc_compress open file node allows more than once

[b_lkasam]

On 2017-11-27 11:20, Vinod Koul wrote:
> On Thu, Nov 23, 2017 at 02:41:00PM +0100, Takashi Iwai wrote:
>> On Thu, 23 Nov 2017 14:18:30 +0100,
>> b_lkasam@codeaurora.org wrote:
>> >
>> > Hi ALSA team,
>> > This is regarding compress offload framework file operations wrt open
>> > system call.
>> >
>> > For open call to this driver currently allows for same node more than
>> > once.
>> >
>> > This may result in memleak issues or data corruption if same node in
>> > use by multiple instances.
>> 
>> This only depends on the driver side implementation.  There is no
>> restriction in the core side, as the multiple open is possible
>> depending on the hardware design and the driver implementation.
> 
> sounds right to me, also is the leak on core side or your driver?
> 
Sorry for delay in getting back.. Yes, leak was in driver not on core 
side.
Thanks everyone for inputs...We have fixed it in our driver.
>> 
>> > can we have restriction to avoid open of same node using some refcount?
>> >
>> > we can either have refcount under compr->private_data and cross check
>> > in below function before proceeding.
>> > static int snd_compr_open(struct inode *inode, struct file *f)
>> >
>> > Please suggest accordingly.
>> 
>> I'd suggest just to add an exclusive open check in the driver side.
>> The open/close callbacks are protected via a mutex, so it can be a
>> simple bool flag check.
>> 
>> If the exclusive access is regarded as a common pattern, we can add
>> some flag to snd_compr later and check it in the core, instead, too.