Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: lSo//Xilr/84uDypFzmhOjvDZz7NkFPqU6OhizK4d8dXQfz4SkJI4hZSSgsVDsbhLYMf4ZK3xt
 YCvRrc4KIBJp2lZzexwD9oMKNAnCLhWn/ezE49EgB7vQC19Rhzmb6thLQUOLWKGDGgfCBYDTEo
 stzdM+LaIHH0vCTIWZaEc6W2j2z3nOle3i7WKM7dOxcG4Xo56dDnrdt0+OSiX49xNTNYPdSu8a
 oH8jiUFVLg2/p4yDjMUMZNz9XIW94e1r9uUVEqdoD052rzGNErJHUfrQUsotZgr26wb8utDh7D
 UgQ=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Colin Ian King <colin.king@canonical.com>
CC:     Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@redhat.com>,
        "dm-devel@redhat.com" <dm-devel@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: dm: Forbid requeue of writes to zones
Thread-Topic: dm: Forbid requeue of writes to zones
Thread-Index: AQHXWMY2crVy6LmSRk+Dbf+Tih0t2g==
Date:   Fri, 4 Jun 2021 00:08:47 +0000
Message-ID: <DM6PR04MB708127E21C618C8F5F6B3DB6E73B9@DM6PR04MB7081.namprd04.prod.outlook.com>
References: <7e7530a9-7939-2ad6-bfe1-d3aeeeed1f77@canonical.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 49b71c6f-3832-4ec1-5b8c-08d926ececbe
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Jun 2021 00:08:47.4705
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Tqm2tSZr4khb6BmC/T0NaBccCSVNRl0Q4VVvnpkLd3iI2B4lySV3kh0cabXHHBQbs1X6m/Vk+gLTK7e8iBSaFg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5163
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/06/04 7:17, Colin Ian King wrote:=0A=
> Hi,=0A=
> =0A=
> Static analysis with Coverity on Linux next has found and issue in=0A=
> drivers/md/dm.c with the following commit:=0A=
> =0A=
> commit 2c243153d1d4be4e23735cd10984ac17c7a54531=0A=
> Author: Damien Le Moal <damien.lemoal@wdc.com>=0A=
> Date:   Wed May 26 06:24:58 2021 +0900=0A=
> =0A=
>     dm: Forbid requeue of writes to zones=0A=
> =0A=
> The analysis is as follows:=0A=
> =0A=
>  828 static void dec_pending(struct dm_io *io, blk_status_t error)=0A=
>  829 {=0A=
>  830        unsigned long flags;=0A=
>  831        blk_status_t io_error;=0A=
> =0A=
>     1. var_decl: Declaring variable bio without initializer.=0A=
=0A=
This one is related to #7.=0A=
=0A=
> =0A=
>  832        struct bio *bio;=0A=
>  833        struct mapped_device *md =3D io->md;=0A=
>  834=0A=
>  835        /* Push-back supersedes any I/O errors */=0A=
> =0A=
>     2. Condition !!error, taking true branch.=0A=
> =0A=
>  836        if (unlikely(error)) {=0A=
>  837                spin_lock_irqsave(&io->endio_lock, flags);=0A=
> =0A=
>     3. Condition io->status =3D=3D 11 /* (blk_status_t)11 */, taking fals=
e=0A=
> branch.=0A=
> =0A=
>  838                if (!(io->status =3D=3D BLK_STS_DM_REQUEUE &&=0A=
> __noflush_suspending(md)))=0A=
>  839                        io->status =3D error;=0A=
>  840                spin_unlock_irqrestore(&io->endio_lock, flags);=0A=
>  841        }=0A=
>  842=0A=
=0A=
My patch does not touch these hunks. They are as is. So that is not new.=0A=
=0A=
> =0A=
>     4. Condition atomic_dec_and_test(&io->io_count), taking true branch.=
=0A=
> =0A=
>  843        if (atomic_dec_and_test(&io->io_count)) {=0A=
> =0A=
>     5. Condition io->status =3D=3D 11 /* (blk_status_t)11 */, taking true=
=0A=
> branch.=0A=
> =0A=
>  844                if (io->status =3D=3D BLK_STS_DM_REQUEUE) {=0A=
>  845                        /*=0A=
>  846                         * Target requested pushing back the I/O.=0A=
>  847                         */=0A=
>  848                        spin_lock_irqsave(&md->deferred_lock, flags);=
=0A=
> =0A=
>     6. Condition __noflush_suspending(md), taking true branch.=0A=
> =0A=
>  849                        if (__noflush_suspending(md) &&=0A=
=0A=
I do not understand this one, nor #4.=0A=
=0A=
> =0A=
> Uninitialized pointer read=0A=
>     7. uninit_use_in_call: Using uninitialized value bio when calling=0A=
> dm_is_zone_write.=0A=
> =0A=
>  850                            !WARN_ON_ONCE(dm_is_zone_write(md, bio)))=
=0A=
>  851                                /* NOTE early return due to=0A=
> BLK_STS_DM_REQUEUE below */=0A=
>  852                                bio_list_add_head(&md->deferred,=0A=
> io->orig_bio);=0A=
=0A=
The kernel build robot signaled this one already. Will send an incremental =
patch=0A=
asap today.=0A=
=0A=
> =0A=
> The pointer bio is not initialized and yet is being used in the call to=
=0A=
> function dm_is_zone_write where pointer bio is being accessed. I'm not=0A=
> sure what the original intent was, but this looks incorrect.=0A=
> =0A=
> Colin=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=