From: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
To: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: linux-bcache-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
"Martin K. Petersen"
<martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>,
tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Subject: Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
Date: Tue, 2 Oct 2012 15:07:47 -0700 [thread overview]
Message-ID: <20121002220744.GW26488@google.com> (raw)
In-Reply-To: <20121002215845.GB14471-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Oct 02, 2012 at 05:58:45PM -0400, Vivek Goyal wrote:
> On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote:
> > I'm honestly not sure what your complaint about my bugfix patch was -
> > it's small and complete, it does fix the bug. I don't follow why you
> > think we need to introduce the bip->bio_vec pointer early...
>
> I think having iv1 and iv2 and then not even accessing these using
> bp->iv1 and bp->iv2 is a bad idea even for bugfix.
>
> I have never seen a code which says, hey I have defined two fields in a
> struct but, don't access those fields directly(as there might be padding
> issues). These fields are just there for blocking a chunk of memory but are
> never meant to be accessed directly. I think, that's what my issue is. It
> is bad programming (does not matter whether it is bug fix or not).
>
> For your series it probably is still fine as you will overide it pretty
> soon but what about stable. Anybody looking at that code might want
> to say, hey why not directly initialize bp->iv1 instead of trying to
> do *bp->bip1.bip_vec. And everybody will say, yes looks fine and boom
> a bug is introduced because we did bad programming.
Ok. It's definitely a bit weird and unusual, and if I wasn't getting rid
of it in the next patch it would definitely merit a comment.
For stable... wtf would they be making that kind of change for, and
without reading the relevant code?
Eh, maybe I will stick in that comment and take it out in the next
patch.
WARNING: multiple messages have this Message-ID (diff)
From: Kent Overstreet <koverstreet@google.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org,
dm-devel@redhat.com, axboe@kernel.dk,
"Martin K. Petersen" <martin.petersen@oracle.com>,
tj@kernel.org
Subject: Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split()
Date: Tue, 2 Oct 2012 15:07:47 -0700 [thread overview]
Message-ID: <20121002220744.GW26488@google.com> (raw)
In-Reply-To: <20121002215845.GB14471@redhat.com>
On Tue, Oct 02, 2012 at 05:58:45PM -0400, Vivek Goyal wrote:
> On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote:
> > I'm honestly not sure what your complaint about my bugfix patch was -
> > it's small and complete, it does fix the bug. I don't follow why you
> > think we need to introduce the bip->bio_vec pointer early...
>
> I think having iv1 and iv2 and then not even accessing these using
> bp->iv1 and bp->iv2 is a bad idea even for bugfix.
>
> I have never seen a code which says, hey I have defined two fields in a
> struct but, don't access those fields directly(as there might be padding
> issues). These fields are just there for blocking a chunk of memory but are
> never meant to be accessed directly. I think, that's what my issue is. It
> is bad programming (does not matter whether it is bug fix or not).
>
> For your series it probably is still fine as you will overide it pretty
> soon but what about stable. Anybody looking at that code might want
> to say, hey why not directly initialize bp->iv1 instead of trying to
> do *bp->bip1.bip_vec. And everybody will say, yes looks fine and boom
> a bug is introduced because we did bad programming.
Ok. It's definitely a bit weird and unusual, and if I wasn't getting rid
of it in the next patch it would definitely merit a comment.
For stable... wtf would they be making that kind of change for, and
without reading the relevant code?
Eh, maybe I will stick in that comment and take it out in the next
patch.
next prev parent reply other threads:[~2012-10-02 22:07 UTC|newest]
Thread overview: 122+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-24 22:34 [PATCH v3 00/26] Prep work for immutable bio vecs Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity_split() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
[not found] ` <1348526106-17074-2-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-01 21:23 ` [dm-devel] " Vivek Goyal
2012-10-01 21:23 ` Vivek Goyal
[not found] ` <20121001212336.GA17165-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-01 21:36 ` Kent Overstreet
2012-10-01 21:36 ` Kent Overstreet
2012-10-01 21:42 ` Kent Overstreet
2012-10-01 21:42 ` Kent Overstreet
[not found] ` <20121001214241.GE26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 14:08 ` Vivek Goyal
2012-10-02 14:08 ` Vivek Goyal
[not found] ` <20121002140847.GD758-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:26 ` Kent Overstreet
2012-10-02 20:26 ` Kent Overstreet
[not found] ` <20121002202643.GQ26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:32 ` Vivek Goyal
2012-10-02 20:32 ` Vivek Goyal
2012-10-02 21:01 ` Kent Overstreet
2012-10-02 21:01 ` [dm-devel] " Kent Overstreet
[not found] ` <20121002210143.GT26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 21:58 ` Vivek Goyal
2012-10-02 21:58 ` Vivek Goyal
[not found] ` <20121002215845.GB14471-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 22:07 ` Kent Overstreet [this message]
2012-10-02 22:07 ` Kent Overstreet
2012-10-02 22:30 ` Martin K. Petersen
2012-09-24 22:34 ` [PATCH v3 02/26] block: Convert integrity to bvec_alloc_bs() Kent Overstreet
2012-10-02 15:12 ` [dm-devel] " Vivek Goyal
2012-10-02 20:52 ` Kent Overstreet
2012-10-02 20:52 ` [dm-devel] " Kent Overstreet
[not found] ` <20121002205249.GR26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 22:05 ` Vivek Goyal
2012-10-02 22:05 ` Vivek Goyal
2012-10-02 22:17 ` Kent Overstreet
[not found] ` <1348526106-17074-3-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 15:37 ` Vivek Goyal
2012-10-02 15:37 ` Vivek Goyal
2012-10-02 21:00 ` Kent Overstreet
[not found] ` <20121002210006.GS26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 22:02 ` Vivek Goyal
2012-10-02 22:02 ` Vivek Goyal
2012-09-24 22:34 ` [PATCH v3 03/26] block: Add bio_advance() Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 04/26] block: Refactor blk_update_request() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-10-02 18:43 ` [dm-devel] " Vivek Goyal
[not found] ` <20121002184359.GC3283-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:10 ` Kent Overstreet
2012-10-02 20:10 ` Kent Overstreet
2012-10-02 20:14 ` Vivek Goyal
[not found] ` <20121002201451.GH758-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:25 ` Kent Overstreet
2012-10-02 20:25 ` Kent Overstreet
2012-10-02 18:59 ` Vivek Goyal
2012-10-02 18:59 ` [dm-devel] " Vivek Goyal
[not found] ` <20121002185955.GD3283-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:09 ` Kent Overstreet
2012-10-02 20:09 ` Kent Overstreet
[not found] ` <1348526106-17074-1-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-24 22:34 ` [PATCH v3 05/26] md: Convert md_trim_bio() to use bio_advance() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
[not found] ` <1348526106-17074-6-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-26 0:38 ` NeilBrown
2012-09-26 0:38 ` NeilBrown
[not found] ` <20120926103827.4d880cf4-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2012-09-27 4:41 ` Kent Overstreet
2012-09-27 4:41 ` Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 23/26] block: Add bio_alloc_pages() Kent Overstreet
2012-09-24 22:35 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 06/26] block: Add bio_end_sector() Kent Overstreet
2012-09-24 22:34 ` [Drbd-dev] " Kent Overstreet
[not found] ` <1348526106-17074-7-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-25 11:54 ` Lars Ellenberg
2012-09-25 11:54 ` [Drbd-dev] " Lars Ellenberg
2012-09-25 11:54 ` Lars Ellenberg
[not found] ` <20120925115452.GF8143-w1SgEEioFePxa46PmUWvFg@public.gmane.org>
2012-09-25 22:06 ` Kent Overstreet
2012-09-25 22:06 ` Kent Overstreet
[not found] ` <20120925220624.GC22647-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-26 15:16 ` Lars Ellenberg
2012-09-26 15:16 ` Lars Ellenberg
2012-09-26 15:16 ` [Drbd-dev] " Lars Ellenberg
2012-10-02 18:10 ` [dm-devel] " Vivek Goyal
2012-10-02 18:10 ` Vivek Goyal
2012-10-02 18:10 ` [Drbd-dev] " Vivek Goyal
[not found] ` <20121002181001.GB3283-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:20 ` Kent Overstreet
2012-10-02 20:20 ` Kent Overstreet
2012-10-02 20:20 ` [Drbd-dev] " Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 07/26] block: Use bio_sectors() more consistently Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
[not found] ` <1348526106-17074-8-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-24 23:04 ` Jim Paris
2012-09-24 23:04 ` Jim Paris
[not found] ` <20120924230449.GA2040-SRSuHwkuBJlaX0KmTac7FA@public.gmane.org>
2012-09-24 23:09 ` Kent Overstreet
2012-09-24 23:09 ` Kent Overstreet
2012-09-25 0:54 ` Ed Cashin
2012-09-24 22:34 ` [PATCH v3 08/26] block: Change bio_split() to respect the current value of bi_idx Kent Overstreet
2012-09-24 22:34 ` [Drbd-dev] " Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 09/26] block: Remove bi_idx references Kent Overstreet
[not found] ` <1348526106-17074-10-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 19:24 ` [dm-devel] " Vivek Goyal
2012-10-02 19:24 ` Vivek Goyal
2012-10-02 20:16 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 10/26] block: Remove some unnecessary bi_vcnt usage Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 11/26] block: Add submit_bio_wait(), remove from md Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
[not found] ` <1348526106-17074-12-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-25 5:51 ` [dm-devel] " Hannes Reinecke
2012-09-25 5:51 ` Hannes Reinecke
2012-09-25 22:15 ` Kent Overstreet
2012-10-02 19:41 ` Vivek Goyal
2012-10-02 19:41 ` Vivek Goyal
[not found] ` <20121002194132.GF3283-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:11 ` Kent Overstreet
2012-10-02 20:11 ` Kent Overstreet
[not found] ` <20121002201105.GL26488-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:16 ` Vivek Goyal
2012-10-02 20:16 ` Vivek Goyal
[not found] ` <20121002201630.GI758-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-10-02 20:22 ` Kent Overstreet
2012-10-02 20:22 ` Kent Overstreet
2012-10-04 6:07 ` Hannes Reinecke
2012-09-24 22:34 ` [PATCH v3 12/26] raid10: Use bio_reset() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 13/26] raid1: use bio_reset() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 14/26] raid5: " Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 15/26] raid1: Refactor narrow_write_error() to not use bi_idx Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 16/26] block: Add bio_copy_data() Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 17/26] pktcdvd: use bio_copy_data() Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 18/26] pktcdvd: Use bio_reset() in disabled code to kill bi_idx usage Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:34 ` [PATCH v3 19/26] raid1: use bio_copy_data() Kent Overstreet
2012-09-24 22:34 ` Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 20/26] bounce: Refactor __blk_queue_bounce to not use bi_io_vec Kent Overstreet
2012-09-24 22:35 ` Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 21/26] block: Add bio_for_each_segment_all() Kent Overstreet
2012-09-24 22:35 ` Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 22/26] block: Convert some code to bio_for_each_segment_all() Kent Overstreet
2012-09-24 22:35 ` Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 24/26] block: Add an explicit bio flag for bios that own their bvec Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 25/26] bio-integrity: Add explicit field for owner of bip_buf Kent Overstreet
2012-09-24 22:35 ` [PATCH v3 26/26] block: Add BIO_SUBMITTED flag, kill BIO_CLONED Kent Overstreet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121002220744.GW26488@google.com \
--to=koverstreet-hpiqsd4aklfqt0dzr+alfa@public.gmane.org \
--cc=axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org \
--cc=dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=linux-bcache-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org \
--cc=tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.