linux-kernel - Re: [PATCH] relay: Add global mode support for buffer-only channels

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: <b905c0b6-a295-882f-9714-cf259119e96c@intel.com>
Date:	Mon, 4 Jul 2016 18:33:08 +0530
From:	"Goel, Akash" <akash.goel@...el.com>
To:	Chris Wilson <chris@...is-wilson.co.uk>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	viro@...iv.linux.org.uk,
	Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>,
	Tom Zanussi <tzanussi@...il.com>, akash.goel@...el.com
Subject: Re: [PATCH] relay: Add global mode support for buffer-only channels



On 7/4/2016 1:30 PM, Chris Wilson wrote:
> On Sun, Jul 03, 2016 at 09:45:27PM +0530, akash.goel@...el.com wrote:
>> From: Akash Goel <akash.goel@...el.com>
>>
>> The following patch added support to use channels with no associated files.
>> 	relay: add buffer-only channels; useful for early logging
>> This is useful when the exact location of relay file is not known or the
>> the parent directory of relay file is not available, while creating the
>> channel and the logging has to start right from the boot.
>>
>> But there was no provision to use global mode with buffer-only channels,
>> which is added by this patch, without modifying the interface where initially
>> there will be a dummy invocation of create_buf_file callback through which
>> kernel client can convey the need of a global buffer.
>>
>> For the use case where drivers/kernel clients want a simple interface for the
>> userspace, which enables them to capture data/logs from relay file in order &
>> without any post processing, support of Global buffer mode is warranted.
>>
>> Cc: Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>
>> Cc: Tom Zanussi <tzanussi@...il.com>
>> Cc: Chris Wilson <chris@...is-wilson.co.uk>
>> Signed-off-by: Akash Goel <akash.goel@...el.com>
>> ---
>>  kernel/relay.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/kernel/relay.c b/kernel/relay.c
>> index 04d7cf3..b267384 100644
>> --- a/kernel/relay.c
>> +++ b/kernel/relay.c
>> @@ -451,6 +451,16 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
>>  		if (!dentry)
>>  			goto free_buf;
>>  		relay_set_buf_dentry(buf, dentry);
>> +	} else {
>
> The trailing block can be replaced with:
>
> 		/* Only retrieve global info, nothing more, nothing less */
>

Thanks, will do like this.
>> +		dentry = chan->cb->create_buf_file(NULL, NULL,
>> +					S_IRUSR, buf,
>> +					&chan->is_global);
>
> Watch alignment, align to the opening bracket.
>
>> +		if (WARN_ON(dentry))
>> +			goto free_buf;
>>  	}
>>
>>   	buf->cpu = cpu;
>> @@ -666,6 +676,27 @@ int relay_late_setup_files(struct rchan *chan,
>>  	}
>>  	chan->has_base_filename = 1;
>>  	chan->parent = parent;
>> +
>> +	if (chan->is_global) {
>> +		if (unlikely(!chan->buf[0])) {
>
> if (WARN_ON_ONCE(!chan->buf[0])) {
>
>> +			WARN_ONCE(1, KERN_ERR "CPU 0 has no buffer!\n");
>
> The WARN includes the unlikely and the message here isn't any more
> informative than the line itself, plus has the erroneous KERN_ERR
>
Thanks will combine the WARN and if check.

>> +			mutex_unlock(&relay_channels_mutex);
>> +			return -EINVAL;
>> +                }
>> +
>> +		dentry = relay_create_buf_file(chan, chan->buf[0], 0);
>> +
>> +		if (unlikely(!dentry))
>> +			err = -EINVAL;
>> +		else if (WARN_ON(!chan->is_global))
>
> ?
>

Though have checked 'is_global' value before, but can't rule out the
clients toggling its value (inadvertently or maliciously).

Actually create_buf_file() callback will be invoked again from the
relay_create_buf_file() and pointer of 'chan->is_global' is passed in
that.
So to be on safer side, thought to validate the 'chan->is_global' value 
after the call to relay_create_buf_file().

>> +			err = -EINVAL;
>> +		else
>> +			relay_set_buf_dentry(chan->buf[0], dentry);
>> +
>> +		mutex_unlock(&relay_channels_mutex);
>> +		return err;
>
> This could be written:
>
Thanks this is much better.


Best Regards
Akash

> if (chan->is_global) {
> 	err = -EINVAL;
> 	if (!WARN_ON_ONCE(!chan->buf[0])) {
> 		dentry = relay_create_buf_file(chan, chan->buf[0], 0))
> 		if (dentry) {
> 			relay_set_buf_dentry(chan->buf[0], dentry);
> 			err = 0;
> 		}
> 	}
> 	mutex_unlock();
> 	return err;
> }
>
> for a single exit/unlock path in this block.
> -Chris
>