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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

regards,
dan