From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6FBCB1173C for ; Tue, 12 Dec 2023 23:46:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="gwJ8APWw" Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC954C433C9; Tue, 12 Dec 2023 23:46:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1702424762; bh=pGrGsb6BqxBVwTK3LkmsdHZprxehakyiO1idycywAO0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=gwJ8APWwAUn/s8Kn8zl7ScvgcxqA/yEKePvL7j0R1K7INopMzTXpKrtOv4A0JO5nj m11Hsk+B6NaVlH7AHuOxZFE+uQiKOviAP/APCmS4OFBIxyuFK0uQRPf4jP/hlJe/Nz QfsgDfyc9fqy8Kvw3EnKkUnJECSQfgSdsrYSuFkCV9CblO46Q4giWeRf7jFMgd5mkb LIlH/UWhYLQmg7V9fZ0PROuM5FXzUrScDc+ic/xbPOeNsjCJFPtxT76iCzYT35pUQR TnCk8LV+bl7wdQ/R24wPhxndUGdpulAvjSh2Uk3ZzfPTTd7nmVJaQBuGKQVio6YWdn /2Reh/5NTEf/w== Date: Wed, 13 Dec 2023 08:45:58 +0900 From: Masami Hiramatsu (Google) To: Steven Rostedt Cc: LKML , Linux Trace Kernel , Masami Hiramatsu , Mark Rutland , Mathieu Desnoyers , Kent Overstreet Subject: Re: [PATCH v3] ring-buffer: Fix writing to the buffer with max_data_size Message-Id: <20231213084558.90dc9a9064c48cebb3d0ab8f@kernel.org> In-Reply-To: <20231212111617.39e02849@gandalf.local.home> References: <20231212111617.39e02849@gandalf.local.home> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-trace-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Tue, 12 Dec 2023 11:16:17 -0500 Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > The maximum ring buffer data size is the maximum size of data that can be > recorded on the ring buffer. Events must be smaller than the sub buffer > data size minus any meta data. This size is checked before trying to > allocate from the ring buffer because the allocation assumes that the size > will fit on the sub buffer. > > The maximum size was calculated as the size of a sub buffer page (which is > currently PAGE_SIZE minus the sub buffer header) minus the size of the > meta data of an individual event. But it missed the possible adding of a > time stamp for events that are added long enough apart that the event meta > data can't hold the time delta. > > When an event is added that is greater than the current BUF_MAX_DATA_SIZE > minus the size of a time stamp, but still less than or equal to > BUF_MAX_DATA_SIZE, the ring buffer would go into an infinite loop, looking > for a page that can hold the event. Luckily, there's a check for this loop > and after 1000 iterations and a warning is emitted and the ring buffer is > disabled. But this should never happen. > > This can happen when a large event is added first, or after a long period > where an absolute timestamp is prefixed to the event, increasing its size > by 8 bytes. This passes the check and then goes into the algorithm that > causes the infinite loop. > > For events that are the first event on the sub-buffer, it does not need to > add a timestamp, because the sub-buffer itself contains an absolute > timestamp, and adding one is redundant. > > The fix is to check if the event is to be the first event on the > sub-buffer, and if it is, then do not add a timestamp. > > This also fixes 32 bit adding a timestamp when a read of before_stamp or > write_stamp is interrupted. There's still no need to add that timestamp if > the event is going to be the first event on the sub buffer. > > Also, if the buffer has "time_stamp_abs" set, then also check if the > length plus the timestamp is greater than the BUF_MAX_DATA_SIZE. > > Link: https://lore.kernel.org/all/20231212104549.58863438@gandalf.local.home/ > Link: https://lore.kernel.org/linux-trace-kernel/20231212071837.5fdd6c13@gandalf.local.home > > Cc: stable@vger.kernel.org > Fixes: a4543a2fa9ef3 ("ring-buffer: Get timestamp after event is allocated") > Fixes: 58fbc3c63275c ("ring-buffer: Consolidate add_timestamp to remove some branches") > Reported-by: Kent Overstreet # (on IRC) > Signed-off-by: Steven Rostedt (Google) This looks good to me :) Acked-by: Masami Hiramatsu (Google) Thank you! > --- > Changes since v2: https://lore.kernel.org/linux-trace-kernel/20231212065922.05f28041@gandalf.local.home > > - Just test 'w' first, and then do the rest of the checks. > > kernel/trace/ring_buffer.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 8d2a4f00eca9..b8986f82eccf 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -3579,7 +3579,10 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer, > * absolute timestamp. > * Don't bother if this is the start of a new page (w == 0). > */ > - if (unlikely(!a_ok || !b_ok || (info->before != info->after && w))) { > + if (!w) { > + /* Use the sub-buffer timestamp */ > + info->delta = 0; > + } else if (unlikely(!a_ok || !b_ok || info->before != info->after)) { > info->add_timestamp |= RB_ADD_STAMP_FORCE | RB_ADD_STAMP_EXTEND; > info->length += RB_LEN_TIME_EXTEND; > } else { > @@ -3737,6 +3740,8 @@ rb_reserve_next_event(struct trace_buffer *buffer, > if (ring_buffer_time_stamp_abs(cpu_buffer->buffer)) { > add_ts_default = RB_ADD_STAMP_ABSOLUTE; > info.length += RB_LEN_TIME_EXTEND; > + if (info.length > BUF_MAX_DATA_SIZE) > + goto out_fail; > } else { > add_ts_default = RB_ADD_STAMP_NONE; > } > -- > 2.42.0 > -- Masami Hiramatsu (Google)