linux-kernel - Re: [PATCH v2 1/6] media: add media token device resource framework

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: <543F1708.1040303@osg.samsung.com>
Date:	Wed, 15 Oct 2014 18:53:28 -0600
From:	Shuah Khan <shuahkh@....samsung.com>
To:	Takashi Iwai <tiwai@...e.de>
CC:	m.chehab@...sung.com, akpm@...ux-foundation.org,
	gregkh@...uxfoundation.org, crope@....fi, olebowle@....com,
	dheitmueller@...nellabs.com, hverkuil@...all.nl,
	ramakrmu@...co.com, sakari.ailus@...ux.intel.com,
	laurent.pinchart@...asonboard.com, perex@...ex.cz,
	prabhakar.csengg@...il.com, tim.gardner@...onical.com,
	linux@...elenboom.it, linux-media@...r.kernel.org,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] media: add media token device resource framework

On 10/15/2014 11:05 AM, Takashi Iwai wrote:

>> +#if defined(CONFIG_MEDIA_SUPPORT)
>> +extern int media_tknres_create(struct device *dev);
>> +extern int media_tknres_destroy(struct device *dev);
>> +
>> +extern int media_get_tuner_tkn(struct device *dev);
>> +extern int media_put_tuner_tkn(struct device *dev);
>> +
>> +extern int media_get_audio_tkn(struct device *dev);
>> +extern int media_put_audio_tkn(struct device *dev);
> 
> The words "tknres" and "tkn" (especially latter) look ugly and not
> clear to me.  IMO, it should be "token".

No problem. I can change that to media_token_create/destroy()
and expand token in other functions.

>> +struct media_tkn {
>> +	spinlock_t lock;
>> +	unsigned int owner;	/* owner task pid */
>> +	unsigned int tgid;	/* owner task gid */
>> +	struct task_struct *task;
>> +};
>> +
>> +struct media_tknres {
>> +	struct media_tkn tuner;
>> +	struct media_tkn audio;
>> +};
> 
> Why do you need to have both tuner and audio tokens?  If I understand
> correctly, no matter whether it's tuner or audio, if it's being used,
> the open must fail, right?

As it evolved during development, it turns out at the moment I don't
have any use-cases that require holding audio and tuner separately.
It probably could be collapsed into just a media token. I have to
think about this some.

>> +
>> +static int __media_get_tkn(struct media_tkn *tkn, char *tkn_str)
>> +{
>> +	int rc = 0;
>> +	unsigned tpid;
>> +	unsigned tgid;
>> +
>> +	spin_lock(&tkn->lock);
> 
> You should use spin_lock_irqsave() here and in all other places.
> The trigger callback in usb-audio, for example, may be called in irq
> context.

ok. Good point, will change that.

> 
>> +
>> +	tpid = task_pid_nr(current);
>> +	tgid = task_tgid_nr(current);
>> +
>> +	/* allow task in the same group id to release */
> 
> IMO, it's not "release" but "steal"...  But what happens if the stolen
> owner calls put?  Then it'll be released although the stealing owner
> still thinks it's being held.

Yeah it could be called a steal. :) Essentially tgid happens to be
the real owner. I am overwriting the pid with current pid when
tgid is same.

media dvb and v4l apps start two or more threads that all share the
tgid and subsequent token gets should be allowed based on the tgid.

Scenario 1:

Please note that there are 3 device files in question and media
token resource is the lock for all. Hence the changes to v4l-core,
dvb-core, and snd-usb-audio to hold the token for exclusive access
when the task or tgid don't match.

program starts:
- first thread opens device file in R/W mode - open gets the token
  or thread makes ioctls calls that clearly indicates intent to
  change tuner settings
- creates one thread for audio
- creates another for video or continues video function
- video thread does a close and re-opens the device file

  In this case when thread tries to close, token put fails unless
  tasks with same tgid are allowed to release.
  ( I ran into this one of the media applications and it is a valid
    case to handle, thread can close the file and should be able to
    open again without running into token busy case )

- or continue to just use the device file
- audio thread does snd_pcm_capture ops - trigger start

program exits:
- video thread closes the device file
- audio thread does snd_pcm_capture ops - trigger stop

This got me thinking about the scenario when an application
does a fork() as opposed to create a thread. I have to think
about this and see how I can address that.

> 
>> +	if (tkn->task && ((tkn->task != current) && (tkn->tgid != tgid))) {
> 
> Shouldn't the second "&&" be "||" instead?
> And too many parentheses there.

Right - this is my bad. The comment right above this conditional
is a give away that, at some point I did a copy and paste from
_put to _get and only changed the first task null check.
I am yelling at myself at the moment.

> 
>> +			rc = -EBUSY;
> 
> Wrong indentation.

Yes. Will fix that.

> 
> I have a feeling that the whole thing can be a bit more simplified in
> the end...
> 

Any ideas to simplify are welcome.

thanks,
-- Shuah




-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@....samsung.com | (970) 217-8978
--
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/