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]
Date:	Mon, 10 Nov 2014 19:33:29 +0100
From:	Petr Mladek <pmladek@...e.cz>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC][PATCH 08/12 v3] tracing: Add seq_buf_get_buf() and
 seq_buf_commit() helper functions

On Fri 2014-11-07 13:39:29, Steven Rostedt wrote:
> More updates. Hmm, maybe I should have posted the full series ;-)
> 
> -- Steve
> 
> From 41a3f3f5e772ca26ef4441a0312d3f108693d7dc Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
> Date: Wed, 29 Oct 2014 17:30:50 -0400
> Subject: [PATCH] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper
>  functions
> 
> Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
> are used by seq_buf_path(). This makes the code similar to the
> seq_file: seq_path() function, and will help to be able to consolidate
> the functions between seq_file and trace_seq.
> 
> Link: http://lkml.kernel.org/r/20141104160222.644881406@goodmis.org
> 
> Tested-by: Jiri Kosina <jkosina@...e.cz>
> Acked-by: Jiri Kosina <jkosina@...e.cz>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>

Reviewed-by: Petr Mladek <pmladek@...e.cz>

Well, I am curious about the BUG_ONs, see below.

> ---
>  include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  kernel/trace/seq_buf.c  |  7 +++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 4aab47d10760..7dacdc791225 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -61,6 +61,46 @@ seq_buf_buffer_left(struct seq_buf *s)
>  	return s->size - s->len;
>  }
>  
> +/**
> + * seq_buf_get_buf - get buffer to write arbitrary data to
> + * @s: the seq_buf handle
> + * @bufp: the beginning of the buffer is stored here
> + *
> + * Return the number of bytes available in the buffer, or zero if
> + * there's no space.
> + */
> +static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
> +{
> +	BUG_ON(s->len > s->size + 1);

I just wonder if the BUG_ON() is appropriate here. There is used
WARN_ON() for the other similar checks.

On one hand. This function will be used by a code that manipulates
the buffer its own way. Therefore the BUG() would help to debug
potential problems.

On the other hand, this function is used just to get the buffer.
Therefore the BUG() might come too late. The buffer was broken
somewhere else.

> +
> +	if (s->len < s->size) {
> +		*bufp = s->buffer + s->len;
> +		return s->size - s->len;
> +	}
> +
> +	*bufp = NULL;
> +	return 0;
> +}
> +
> +/**
> + * seq_buf_commit - commit data to the buffer
> + * @s: the seq_buf handle
> + * @num: the number of bytes to commit
> + *
> + * Commit @num bytes of data written to a buffer previously acquired
> + * by seq_buf_get.  To signal an error condition, or that the data
> + * didn't fit in the available space, pass a negative @num value.
> + */
> +static inline void seq_buf_commit(struct seq_buf *s, int num)
> +{
> +	if (num < 0) {
> +		seq_buf_set_overflow(s);
> +	} else {
> +		BUG_ON(s->len + num > s->size + 1);

I agree that the BUG_ON makes sense here. If someone passed too big
"num", she probably also wrote too many bytes and the memory is
corrupted at this point.

> +		s->len += num;
> +	}
> +}
> +

Best Regards,
Petr
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ