linux-kernel - Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component

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: <CANLsYkxTiwbE9WM4yR0hThPopv5ESJM0_B0+Qcay=aiTvaeJ0g@mail.gmail.com>
Date:	Fri, 12 Feb 2016 12:02:27 -0700
From:	Mathieu Poirier <mathieu.poirier@...aro.org>
To:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc:	Chunyan Zhang <zhang.chunyan@...aro.org>,
	Mike Leach <mike.leach@....com>,
	Michael Williams <Michael.Williams@....com>,
	Al Grant <al.grant@....com>, "Jeremiassen, Tor" <tor@...com>,
	Nicolas GUION <nicolas.guion@...com>,
	Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
	Pratik Patel <pratikp@...eaurora.org>,
	Jon Corbet <corbet@....net>,
	Mark Rutland <mark.rutland@....com>,
	Lyra Zhang <zhang.lyra@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, linux-api@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH V3 6/6] coresight-stm: adding driver for CoreSight STM component

On 12 February 2016 at 08:28, Alexander Shishkin
<alexander.shishkin@...ux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@...aro.org> writes:
>
>> +static long stm_generic_set_options(struct stm_data *stm_data,
>> +                                 unsigned int master,
>> +                                 unsigned int channel,
>> +                                 unsigned int nr_chans,
>> +                                 unsigned long options)
>> +{
>> +     struct stm_drvdata *drvdata = container_of(stm_data,
>> +                                                struct stm_drvdata, stm);
>> +     if (!(drvdata && drvdata->enable))
>> +             return -EINVAL;
>> +
>> +     if (channel >= drvdata->numsp)
>> +             return -EINVAL;
>> +
>> +     switch (options) {
>> +     case STM_OPTION_GUARANTEED:
>> +             set_bit(channel, drvdata->chs.guaranteed);
>> +             break;
>> +
>> +     case STM_OPTION_INVARIANT:
>> +             clear_bit(channel, drvdata->chs.guaranteed);
>> +             break;
>
> This is a bad interface. Firstly, neither option is described
> anywhere. Secondly, I'm pretty sure "invariant" does not mean "not
> guaranteed" in english, although this function seems to imply this.

Regardless of the semantic associated to the word "invariant" in the
English language, the documentation characterises a channel configured
in best effort delivery mode as "invariant".  This is also the
opposite of the "guaranteed" mode where packets are guaranteed to be
delivered.  Adding a few comments here is probably a good idea.

>
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static long stm_generic_get_options(struct stm_data *stm_data,
>> +                                 unsigned int master,
>> +                                 unsigned int channel,
>> +                                 unsigned int nr_chans,
>> +                                 u64 *options)
>> +{
>> +     struct stm_drvdata *drvdata = container_of(stm_data,
>> +                                                struct stm_drvdata, stm);
>> +     if (!(drvdata && drvdata->enable))
>> +             return -EINVAL;
>> +
>> +     if (channel >= drvdata->numsp)
>> +             return -EINVAL;
>> +
>> +     switch (*options) {
>> +     case STM_OPTION_GUARANTEED:
>> +             *options = test_bit(channel, drvdata->chs.guaranteed);
>> +             break;
>
> This just doesn't work. @options here is an on-stack variable in
> stm_char_ioctl(), hitherto uninitialized. The get_options ioctl command
> as you implemented it doesn't fetch @options from userspace either, it
> just passes a pointer to it to this callback, expecting that the
> callback will set it so that it can be copy_to_user()ed back to the
> user.

Yes, that was the intended behaviour - to allow user space to see in
what mode channels are configured (guaranteed/invariant).

>
> Then, when we figure this out, there is again the question of what
> should one make of STM_OPTION_{GUARANTEED,INVARIANT} and how do they fit
> into *options.

The interface asks if the channel is configured in "guaranteed" mode.
If not and because there isn't another mode available, it is
automatically in "invariant" mode.

But as I pointed out in my earlier email this may no longer needed.
One has to issue a system call anyway, might as well just go ahead
with the configuration request.

Thanks,
Mathieu

>
> The idea behind set_options ioctl is that the user specifies a bit mask
> of options that he/she wants to set.
>
> [snip]
>
>> +#ifndef __UAPI_CORESIGHT_STM_H_
>> +#define __UAPI_CORESIGHT_STM_H_
>> +
>> +#define STM_FLAG_TIMESTAMPED   BIT(3)
>> +#define STM_FLAG_GUARANTEED    BIT(7)
>> +
>> +enum {
>> +     STM_OPTION_GUARANTEED = 0,
>> +     STM_OPTION_INVARIANT,
>> +};
>
> Each of these guys could also use an explanation.
>
> Regards,
> --
> Alex