Re: [bug report] staging: erofs: introduce erofs_grab_bio

[Gao Xiang <gaoxiang25@huawei.com>]

On Wed, Aug 28, 2019 at 02:36:13PM +0300, Dan Carpenter wrote:
> On Wed, Aug 28, 2019 at 07:02:49PM +0800, Gao Xiang wrote:
> > Hi Dan,
> > 
> > On Wed, Aug 28, 2019 at 01:55:41PM +0300, Dan Carpenter wrote:
> > > Hello Gao Xiang,
> > > 
> > > The patch 8be31270362b: "staging: erofs: introduce erofs_grab_bio"
> > > from Aug 21, 2018, leads to the following static checker warning:
> > > 
> > > 	fs/erofs/zdata.c:1272 z_erofs_vle_submit_all()
> > > 	error: 'bio' dereferencing possible ERR_PTR()
> > > 
> > > fs/erofs/zdata.c
> > >   1259                  if (bio && force_submit) {
> > >   1260  submit_bio_retry:
> > >   1261                          __submit_bio(bio, REQ_OP_READ, 0);
> > >   1262                          bio = NULL;
> > >   1263                  }
> > >   1264  
> > >   1265                  if (!bio) {
> > >   1266                          bio = erofs_grab_bio(sb, first_index + i,
> > >   1267                                               BIO_MAX_PAGES, bi_private,
> > >   1268                                               z_erofs_vle_read_endio, true);
> > > 
> > > This assumes erofs_grab_bio() can't fail.  It returns ERR_PTR(-ENOMEM)
> > > on failure.
> > 
> > I think there is no problem at all as well.
> > The last argument of erofs_grab_bio is nofail, and here is "true".
> > 
> > 415 static inline struct bio *erofs_grab_bio(struct super_block *sb,
> > 416                                          erofs_blk_t blkaddr,
> > 417                                          unsigned int nr_pages,
> > 418                                          void *bi_private, bio_end_io_t endio,
> > 419                                          bool nofail)
> > 420 {
> > 421         const gfp_t gfp = GFP_NOIO;
> > 422         struct bio *bio;
> > 423
> > 424         do {
> > 425                 if (nr_pages == 1) {
> > 426                         bio = bio_alloc(gfp | (nofail ? __GFP_NOFAIL : 0), 1);
> > 427                         if (unlikely(!bio)) {
> > 428                                 DBG_BUGON(nofail);
> > 429                                 return ERR_PTR(-ENOMEM);
> > 430                         }
> > 431                         break;
> > 432                 }
> > 433                 bio = bio_alloc(gfp, nr_pages);
> > 434                 nr_pages /= 2;
> > 435         } while (unlikely(!bio));
> > 436
> > 437         bio->bi_end_io = endio;
> > 438         bio_set_dev(bio, sb->s_bdev);
> > 439         bio->bi_iter.bi_sector = (sector_t)blkaddr << LOG_SECTORS_PER_BLOCK;
> > 440         bio->bi_private = bi_private;
> > 441         return bio;
> > 442 }
> > 
> > You can see __GFP_NOFAIL is set, Am I missing something?
> > 
> 
> Ah.  Yes.  You're right.

That is fine :) Thanks,

Cheers,
Gao Xiang

> 
> regards,
> dan 
> 
>