Project

General

Profile

Feature #2706

101: All obtained blk requests should be put after all

Added by Evgeny Novikov almost 7 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Start date:
04/04/2012
Due date:
% Done:

100%

Estimated time:
Published in build:
935d93f

Description

blk requests are obtained (some memory is allocated for them) by means of functions blk_get_request and blk_make_request. After usage (usually with help of blk_execute_rq) these requests should be put (and freed) with help of blk_put_request.
This rule is specific:resource:leaks:blk_request in our classifier and will be 101_1a model.

Links

Sample bugfixes 39a1d13


Related issues

Blocked by Linux Kernel Safety RuleDB - Feature #2707: IS_ERR kernel core function should be modeled because of it is too complex for our verifiers Closed2012-04-04

History

#1 Updated by Evgeny Novikov almost 7 years ago

  • Status changed from Open to Resolved
  • Published in build set to 935d93f

Implemented in commit 935d93f of the master branch. The bug fixed in commit 39a1d13 of linux-stable branch is found both with BLAST and CPAchecker.

#2 Updated by Evgeny Novikov almost 7 years ago

  • Status changed from Resolved to Open

It just finds one concrete bug, so the rule isn't fully implemented.

#3 Updated by Evgeny Novikov almost 7 years ago

Evgeny Novikov wrote:

The bug fixed in commit 39a1d13 of linux-stable branch is found both with BLAST and CPAchecker.

BTW, it requires a special hack to make CPAchecker work:

// HACK for CPAchecker that allows to avoid CIL parsing errors.
around: define(test_bit(nr, addr))
{
0
}

because of
struct net_device {
  ...
  char                    name[IFNAMSIZ];
...
static inline char const *netdev_name(struct net_device const *dev)
{
  ...
  return ( char const *) & ( * dev ) . name;
}

leads to such amazing code after CIL:
__inline static char const   *netdev_name(struct net_device  const  *dev ).
{ 
  ...
  char ( const  (*__cil_tmp7))[16U] ;

  {
  {
#line 2232
  __cil_tmp7 = (char ( const  (*))[16U])dev;
#line 2232
  return ((char const   *)__cil_tmp7);
  }
}
}

#4 Updated by Evgeny Novikov almost 7 years ago

test_bit workaround helps to fix similar issue as described.

#5 Updated by Evgeny Novikov almost 7 years ago

Produced by CIL code isn't a valid C code. So it isn't surprising that it breaks CPAchecker frontend.

#6 Updated by Evgeny Novikov almost 7 years ago

Unfortunately, I cannot make a simple workaround for the given issue (say, with help of aspectator, because it cannot completely remove a function definition). Just manual fix of code allows to avoid it.

#7 Updated by Alexey Khoroshilov almost 7 years ago

  • Subject changed from All obtained blk requests shold be put after all to 101: All obtained blk requests shold be put after all

#8 Updated by Evgeny Novikov almost 7 years ago

  • Priority changed from High to Normal

Reduce priority until we'll decide that it's high actually.

#9 Updated by Evgeny Novikov over 6 years ago

  • Subject changed from 101: All obtained blk requests shold be put after all to 101: All obtained blk requests should be put after all
  • Assignee changed from Evgeny Novikov to Marina Makienko

Indeed this rule is rather promising: it isn't so general as malloc-free, but used in a lot of drivers. That's why it's a good task for Marina. As usual careful investigation, rule and model completion, and thorough testing are required.

#10 Updated by Marina Makienko over 6 years ago

  • Status changed from Open to Resolved
  • % Done changed from 0 to 100

#11 Updated by Marina Makienko over 6 years ago

__blk_put_request() was added in this model.

There was an attempt to check arguments of blk_execute_rq_nowait(): if the last argument is blk_end_sync_rq, it isn't need to call blk_put_request(). But it's a problem to test it.

Also the model should check function call in operation "req->end_io = ...", where req = blk_get_request() or blk_make_request().

#12 Updated by Marina Makienko over 6 years ago

  • Description updated (diff)

#13 Updated by Marina Makienko over 6 years ago

  • Description updated (diff)

#14 Updated by Alexey Khoroshilov about 6 years ago

  • Status changed from Resolved to Open

blk_get_request() and blk_make_request() return error code in different ways.
blk_get_request() returns NULL, blk_make_request() returns ERR_PTR.
Model should support it.

#15 Updated by Alexey Khoroshilov about 6 years ago

Another issue:

    /*Check the last argument of blk_[make|get]_request. If it's one of this values it's isn't necessary to check res on error*/
    if (mask == __GFP_WAIT || mask == GFP_KERNEL || mask == GFP_NOIO)
        /* LDV_COMMENT_CHANGE_STATE Get blk request. */
        ldv_blk_rq = LDV_BLK_RQ_GOT;

Why res is not updated in this branch?

#16 Updated by Marina Makienko about 6 years ago

If the last argument of function (gfp_t gfp_mask) is __GFP_WAIT, we don't need to check function return value. The structure of function blk_get_request() is:

static struct request *get_request(struct request_queue *q, int rw_flags, struct bio *bio, gfp_t gfp_mask) {
    ...
retry:
    rq = __get_request(rl, rw_flags, bio, gfp_mask);
    ...
    if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dead(q))) {
        blk_put_rl(rl);
        return NULL;
    }
    ...
    goto retry;
}

So, if the mask is __GFP_WAIT, function will try to get request again and again.
But if the mask has another value, the model checks blk_get_request() return value.

#17 Updated by Evgeny Novikov almost 6 years ago

Now 5 of 6 false positives are due to the model successfully obtains a request with __GFP_WAIT flags but returns 0. It should return nonzero value in case of blk_get_request() and non pointer error value in case of blk_make_request().

Moreover the model can return a failure without obtaining a request even when functions are called with __GFP_WAIT since a request queue may die (as it said here). Even without verification we can see that there is a number of bugs:
  1. Possible NULL pointer dereference
  2. Possible NULL pointer dereference
  3. Possible NULL pointer dereference
  4. Excessive check of blk_get_request() return value
  5. ...

Developers have ignored patches sent by Marina because of:
drivers/ide is obsolete and scheduled for removal
although Sergei answered:
It's being maintained for bug fixes still, AFAIK
but there are also similar errors in non IDE drivers, e.g. see items 1 and 3 above.

#18 Updated by Evgeny Novikov about 5 years ago

  • Status changed from Open to Resolved

All noted issues with the given specification were eventually fixed in 09b28740 mainly by Alexey. So, now there is just one false positive that it is related with lacks of BLAST.

#19 Updated by Evgeny Novikov about 5 years ago

  • Assignee deleted (Marina Makienko)

#20 Updated by Vadim Mutilin over 4 years ago

  • Description updated (diff)

#21 Updated by Vitaly Mordan over 3 years ago

Does not work on Linux kernel 4.4-rc1.

Error message:

/home/ldvuser/ref_launch/work/current--X--kernel--X--defaultlinux-4.4-rc1.tar.xz--X--101_1a--X--cpachecker/linux-4.4-rc1.tar.xz/csd_deg_dscv/25/dscv_tempdir/rule-instrumentor/101_1a/common-model/ldv_common_model.c: In function 'ldv_blk_get_request':
/home/ldvuser/ref_launch/work/current--X--kernel--X--defaultlinux-4.4-rc1.tar.xz--X--101_1a--X--cpachecker/linux-4.4-rc1.tar.xz/csd_deg_dscv/25/dscv_tempdir/rule-instrumentor/101_1a/common-model/ldv_common_model.c:28:15: error: '__GFP_WAIT' undeclared (first use in this function)

#22 Updated by Vitaly Mordan over 3 years ago

Fixed for 4.4-rc1 according to https://bugzilla.redhat.com/show_bug.cgi?id=1285348.
Currently added in branch fix-101-4.4.

#23 Updated by Alexey Khoroshilov about 3 years ago

There is a bug in the current model:
struct request *ldv_blk_get_request(gfp_t mask) should return ERRPTR instead of NULL.

#24 Updated by Vitaly Mordan about 2 years ago

  • Description updated (diff)

Also available in: Atom PDF