RDMA fixes 2018-06-24

From: Leon Romanovsky <[email protected]>

Flows can be created on UD and RAW_PACKET QP types. Attempts to provide other QP types as an input causes to various unpredictable failures.

The reason to it that in order to support all various types (e.g. XRC), we are supposed to use real_qp handle and not qp handle and give to driver/FW to fail such (XRC) flows. Being valuable solution, the simpler and safer variant is to ban all QP types except UD and RAW_PACKET, instead of relying on driver/FW.

Cc <[email protected]> # 3.11

Fixes 436f2ad05a0b ("IB/core: Export ib_create/destroy_flow through uverbs")

Cc syzkaller <[email protected]>

Reported-by Noa Osherovich <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_cmd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +0 -0

index 779892b63729..c842a9423fbf 100644

From: Leon Romanovsky <[email protected]>

Any error status returned by FW will trigger similar to the following error message in the dmesg.

[   55.884355] mlx5_core 0000:00:04.0: mlx5_cmd_check:712:(pid 555):

ALLOC_UAR(0x802) op_mod(0x0) failed, status limits exceeded(0x8), syndrome (0x0)

Those prints are extremely valuable to diagnose issues with running system and it is important to keep them. However, not-so-careful user can trigger endless number of such prints by depleting HW resources and will spam dmesg.

Rate limiting of such messages solves this issue.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c       | 11 ++++-------
 drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h |  6 ++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

From: Leon Romanovsky <[email protected]>

Number of specs is provided by user and in valid case can be equal to zero. Such argument causes to call to kcalloc() with zero-length request and in return the ZERO_SIZE_PTR is assigned. This pointer is different from NULL and makes various if (..) checks to success.

Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Number of specs is provided by user and in valid case can be equal to zero. Such argument causes to call to kcalloc() with zero-length request and in return the ZERO_SIZE_PTR is assigned. This pointer is different from NULL and makes various if (..) checks to success.

The one seems really weird. There is nothing wrong with ZERO_SIZE_PTR, but this description and fix suggest that something did

ptr = kalloc(0);
ptr[0] = ...;

Which is not allowed of course. Doesn't this mean there is also a missing range check someplace?

Jason

   check_shift_overflow(a, s, d) {
       unsigned _nbits = 8*sizeof(a);
       typeof(a) _a = (a);
       typeof(s) _s = (s);
       typeof(d) _d = (d);

       *_d = ((u64)(_a) << (_s & (_nbits-1)));
       _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
   is_signed_type(a))) != 0);
   }

Quoted 10 lines · Rasmus Villemoes

   check_shift_overflow(a, s, d) {
       unsigned _nbits = 8*sizeof(a);
       typeof(a) _a = (a);
       typeof(s) _s = (s);
       typeof(d) _d = (d);

       *_d = ((u64)(_a) << (_s & (_nbits-1)));
       _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
   is_signed_type(a))) != 0);
   }

Those types are not quite right.. What about this?

    check_shift_overflow(a, s, d) ({
        unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
        typeof(d) _a = a;  // Shift is always performed on type 'd'
        typeof(s) _s = s;
        typeof(d) _d = d;

        *_d = (_a << (_s & (_nbits-1)));

	(((*_d) >> (_s & (_nbits-1)) != _a);
    })

And can we use mathamatcial invertability to prove no overlow and bound _a ? As above.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 15 lines · Jason Gunthorpe

Quoted 10 lines · Rasmus Villemoes

   check_shift_overflow(a, s, d) {
       unsigned _nbits = 8*sizeof(a);
       typeof(a) _a = (a);
       typeof(s) _s = (s);
       typeof(d) _d = (d);

       *_d = ((u64)(_a) << (_s & (_nbits-1)));
       _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
   is_signed_type(a))) != 0);
   }

Those types are not quite right.. What about this?

Quoted 11 lines

    check_shift_overflow(a, s, d) ({
        unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
        typeof(d) _a = a;  // Shift is always performed on type 'd'
        typeof(s) _s = s;
        typeof(d) _d = d;

        *_d = (_a << (_s & (_nbits-1)));

        (((*_d) >> (_s & (_nbits-1)) != _a);
    })

No, because, the check_*_overflow (and the __builtin_*_overflow cousins) functions must do their job without causing undefined behaviour, regardless of what crazy input values and types they are given. Also, the output must be completely defined for all inputs [1]. I omitted it for brevity, but I also wanted a and *d to have the same type, so there should also be one of those (void)(&_a == _d); statements. See the other check_*_overflow and the commit adding them. Without the (u64) cast, any signed (and negative) a would cause UB in your suggestion. Also, having _nbits be 31 when a (and/or *d) has type int, and then and'ing the shift by 30 doesn't make any sense; I have no idea what you're trying to do. I haven't tested the above, but I know from when I wrote the other ones that gcc is smart enough not to actually do the arithmetic in 64 bits when only <= 32 bit types are involved (i.e., gcc sees that the result is anyway implicitly truncated to 32 bits, so only bothers to compute the lower 32 bits).

[1] For this one, it would probably be most consistent to say that the result is a*2^s computed in infinite-precision, then truncated to fit in d. So for too large s, that would just yield 0. But that becomes a bit annoying when s is negative; we don't want to start handling a negative left shift as a right shift. That's also why I said that one should sit down and think about the semantics one really wants, then implement that, and write tests. For a first implementation, it might be completely reasonable to simply say BUILD_BUG_ON(is_signed_type(a)), but that still leaves open what to put in *d when s is negative. But maybe another BUILD_BUG_ON(is_signed_type(s)) could handle that, though that's a bit annoying for integer literals.

And can we use mathamatcial invertability to prove no overlow and

Quoted 2 lines

bound _a ? As above.

It's quite possible that the expression determining whether overflow occured can be written differently, possibly in terms of shifting back, but it definitely needs to return true when s is greater than nbits; check_shift_overflow(1U, 32, &d) must be true. And that expression also must not involve UB.

Rasmus

   On 25 June 2018 at 19:11, Jason Gunthorpe <[1][email protected]> wrote:

     On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:
     >    check_shift_overflow(a, s, d) {
     >        unsigned _nbits = 8*sizeof(a);
     >        typeof(a) _a = (a);
     >        typeof(s) _s = (s);
     >        typeof(d) _d = (d);
     >
     >        *_d = ((u64)(_a) << (_s & (_nbits-1)));
     >        _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
     >    is_signed_type(a))) != 0);
     >    }
     Those types are not quite right.. What about this?
         check_shift_overflow(a, s, d) ({
             unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
             typeof(d) _a = a;  // Shift is always performed on type 'd'
             typeof(s) _s = s;
             typeof(d) _d = d;

             *_d = (_a << (_s & (_nbits-1)));
             (((*_d) >> (_s & (_nbits-1)) != _a);
         })

   No, because, the check_*_overflow (and the __builtin_*_overflow
   cousins) functions must do their job without causing undefined
   behaviour, regardless of what crazy input values and types they are
   given.

Also, the output must be completely defined for all inputs [1]. I omitted it for brevity, but I also wanted a and *d to have the same type, so there should also be one of those (void)(&_a == _d);

statements. See the other check_*_overflow and the commit adding them. Without the (u64) cast, any signed (and negative) a would cause UB in your suggestion.

Also, having _nbits be 31 when a (and/or *d) has type
int, and then and'ing the shift by 30 doesn't make any sense; I have no idea what you're trying to do.

[1] For this one, it would probably be most consistent to say that the result is a*2^s computed in infinite-precision, then truncated to fit in d.

Quoted 11 lines · Jason Gunthorpe

Quoted 1 line · Rasmus Villemoes

   On 25 June 2018 at 19:11, Jason Gunthorpe <[1][email protected]>

Quoted 1 line · wrote:

When thinking about signed cases.. The explicit u64 cast, and implict promotion to typeof(d), produce something counter intuitive, eg:

(u64)(s32)-1 == 0xffffffffffffffff

Quoted 3 lines

Which would result in a shift oucome that is not what anyone would expect, IMHO... So the first version isn't what I'd expect either..

Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint and report no overflow? That's what I'd expect, if negative values are to be supported at all.

OK. The requirement of everything having the same type for the check_*_overflow when gccs builtins are not available was mostly a consequence of my inability to implement completely type-generic versions (but also to enforce some sanity, so people don't do check_add_overflow( s8, size_t, int*)). There's no gcc builtin for shift, but if it's relatively simple to one allowing a and *d to have different types, then why not. It's of course particularly convenient to allow a bare "1" (i.e. int) as a while having *d have some random type.

Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint and report no overflow? That's what I'd expect, if negative values are to be supported at all.

Well, the types you can check at compile-time, the values not, so you still have to define the result, i.e. contents of *d, for negative values (even if we decide that "overflow" should always be signalled in that case).

     What about more like this?
               check_shift_overflow(a, s, d) ({
                   // Shift is always performed on the machine's largest
     unsigned
                   u64 _a = a;
                   typeof(s) _s = s;
                   typeof(d) _d = d;
                   // Make s safe against UB
                   unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
                   *_d = (_a << _to_shift);
                    // s is malformed
                   (_to_shift != _s ||
                    // d is a signed type and became negative
                    *_d < 0 ||
                    // a is a signed type and was negative
                    _a < 0 ||
                    // Not invertable means a was truncated during
     shifting
                    (*_d >> _to_shift) != a))
               })
     I'm not seeing a UB with this?

Something like that might work, but you're not there yet. In particular, your test for whether a is negative is thwarted by using u64 for _a and testing _a < 0...

Quoted 9 lines · Rasmus Villemoes

OK. The requirement of everything having the same type for the check_*_overflow when gccs builtins are not available was mostly a consequence of my inability to implement completely type-generic versions (but also to enforce some sanity, so people don't do check_add_overflow( s8, size_t, int*)). There's no gcc builtin for shift, but if it's relatively simple to one allowing a and *d to have different types, then why not. It's of course particularly convenient to allow a bare "1" (i.e. int) as a while having *d have some random type.

Yes

Quoted 3 lines

Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint and report no overflow? That's what I'd expect, if negative values are to be supported at all.

I would say that is not a desired outcome, bitshift is defined on bits, if the caller wanted something defined as signed multiply they should use multiply.

IMHO, nobody writes 'a << b' expecting sign preservation..

Quoted 4 lines

Well, the types you can check at compile-time, the values not, so you still have to define the result, i.e. contents of *d, for negative values (even if we decide that "overflow" should always be signalled in that case).

Why do a need to define a 'result' beyond whatever the not-undefined behavior shift expression produces?

Quoted 22 lines

     What about more like this?
               check_shift_overflow(a, s, d) ({
                   // Shift is always performed on the machine's largest
     unsigned
                   u64 _a = a;
                   typeof(s) _s = s;
                   typeof(d) _d = d;
                   // Make s safe against UB
                   unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
                   *_d = (_a << _to_shift);
                    // s is malformed
                   (_to_shift != _s ||
                    // d is a signed type and became negative
                    *_d < 0 ||
                    // a is a signed type and was negative
                    _a < 0 ||
                    // Not invertable means a was truncated during
     shifting
                    (*_d >> _to_shift) != a))
               })
     I'm not seeing a UB with this?

Quoted 3 lines

Something like that might work, but you're not there yet. In particular, your test for whether a is negative is thwarted by using u64 for _a and testing _a < 0...

Oops, yes that was intended to be 'a', and of course we need to capture it..

Leon? Seems like agreement, Can you work with this version?

#include <stdint.h>
#include <stdbool.h>
#include <assert.h>

#define u64 uint64_t

/*
* Compute *d = (a << s)
*
* Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
* - 'a << s' causes bits to be lost when stored in d
* - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
* - 'a' is negative
* - 'a << s' sets the sign bit, if any, in '*d'
* *d is not defined if false is returned.
*/

#define check_shift_overflow(a, s, d)                                          \
	({                                                                     \
		typeof(a) _a = a;                                              \
		typeof(s) _s = s;                                              \
		typeof(d) _d = d;                                              \
		u64 _a_full = _a;                                              \
		unsigned int _to_shift =                                       \
			_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
                                                                               \
		*_d = (_a_full << _to_shift);                                  \
                                                                               \
		(_to_shift != _s || *_d < 0 || _a < 0 ||                       \
		 (*_d >> _to_shift) != a);                                     \
	})

int main(int argc, const char *argv[])

{
	int32_t s32;
	uint32_t u32;

	assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
	assert(check_shift_overflow(1, 1, &s32) == false && s32 == (1 << 1));
	assert(check_shift_overflow(1, 30, &s32) == false && s32 == (1 << 30));
	assert(check_shift_overflow(1, 31, &s32) == true);
	assert(check_shift_overflow(1, 32, &s32) == true);
	assert(check_shift_overflow(-1, 1, &s32) == true);
	assert(check_shift_overflow(-1, 0, &s32) == true);

	assert(check_shift_overflow(1, 0, &u32) == false && u32 == (1 << 0));
	assert(check_shift_overflow(1, 1, &u32) == false && u32 == (1 << 1));
	assert(check_shift_overflow(1, 30, &u32) == false && u32 == (1 << 30));
	assert(check_shift_overflow(1, 31, &u32) == false && u32 == (1UL << 31));
	assert(check_shift_overflow(1, 32, &u32) == true);
	assert(check_shift_overflow(-1, 1, &u32) == true);
	assert(check_shift_overflow(-1, 0, &u32) == true);

	assert(check_shift_overflow(0xFFFFFFFF, 0, &u32) == false && u32 == (0xFFFFFFFFUL << 0));
	assert(check_shift_overflow(0xFFFFFFFF, 1, &u32) == true);
	assert(check_shift_overflow(0xFFFFFFFF, 0, &s32) == true);
	assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);
}

Thanks,
Jason

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · Rasmus Villemoes

Well, the types you can check at compile-time, the values not, so you still have to define the result, i.e. contents of *d, for negative values (even if we decide that "overflow" should always be signalled in that case).

Why do a need to define a 'result' beyond whatever the not-undefined behavior shift expression produces?

Quoted 16 lines

/*
* Compute *d = (a << s)
*
* Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
* - 'a << s' causes bits to be lost when stored in d
* - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
* - 'a' is negative
* - 'a << s' sets the sign bit, if any, in '*d'
* *d is not defined if false is returned.
*/

#define check_shift_overflow(a, s, d)                                          \
	({                                                                     \
		typeof(a) _a = a;                                              \
		typeof(s) _s = s;                                              \
		typeof(d) _d = d;                                              \
		u64 _a_full = _a;                                              \

Quoted 8 lines

		unsigned int _to_shift =                                       \
			_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
                                                                               \
		*_d = (_a_full << _to_shift);                                  \
                                                                               \
		(_to_shift != _s || *_d < 0 || _a < 0 ||                       \
		 (*_d >> _to_shift) != a);                                     \
	})

Quoted 6 lines

int main(int argc, const char *argv[])

{
	int32_t s32;
	uint32_t u32;

	assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));

Leon? Seems like agreement, Can you work with this version?

#include <stdint.h>
#include <stdbool.h>
#include <assert.h>

#define u64 uint64_t

/*
* Compute *d = (a << s)
*
* Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
* - 'a << s' causes bits to be lost when stored in d
* - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
* - 'a' is negative
* - 'a << s' sets the sign bit, if any, in '*d'
* *d is not defined if false is returned.
*/

#define check_shift_overflow(a, s, d)                                          \
        ({                                                                     \
                typeof(a) _a = a;                                              \
                typeof(s) _s = s;                                              \
                typeof(d) _d = d;                                              \
                u64 _a_full = _a;                                              \
                unsigned int _to_shift =                                       \
                        _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
                                                                               \
                *_d = (_a_full << _to_shift);                                  \
                                                                               \
                (_to_shift != _s || *_d < 0 || _a < 0 ||                       \
                 (*_d >> _to_shift) != a);                                     \
        })

int main(int argc, const char *argv[])

{
        int32_t s32;
        uint32_t u32;

        assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
        assert(check_shift_overflow(1, 1, &s32) == false && s32 == (1 << 1));
        assert(check_shift_overflow(1, 30, &s32) == false && s32 == (1 << 30));
        assert(check_shift_overflow(1, 31, &s32) == true);
        assert(check_shift_overflow(1, 32, &s32) == true);
        assert(check_shift_overflow(-1, 1, &s32) == true);
        assert(check_shift_overflow(-1, 0, &s32) == true);

        assert(check_shift_overflow(1, 0, &u32) == false && u32 == (1 << 0));
        assert(check_shift_overflow(1, 1, &u32) == false && u32 == (1 << 1));
        assert(check_shift_overflow(1, 30, &u32) == false && u32 == (1 << 30));
        assert(check_shift_overflow(1, 31, &u32) == false && u32 == (1UL << 31));
        assert(check_shift_overflow(1, 32, &u32) == true);

        assert(check_shift_overflow(-1, 1, &u32) == true);
        assert(check_shift_overflow(-1, 0, &u32) == true);

        assert(check_shift_overflow(0xFFFFFFFF, 0, &u32) == false && u32 == (0xFFFFFFFFUL << 0));
        assert(check_shift_overflow(0xFFFFFFFF, 1, &u32) == true);
        assert(check_shift_overflow(0xFFFFFFFF, 0, &s32) == true);
        assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);
}

What about more like this?

          check_shift_overflow(a, s, d) ({

	      // Shift is always performed on the machine's largest unsigned
              u64 _a = a;
	      typeof(s) _s = s;
              typeof(d) _d = d;

	      // Make s safe against UB
	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;

              *_d = (_a << _to_shift);

	       // s is malformed
              (_to_shift != _s ||

	       // d is a signed type and became negative
	       *_d < 0 ||

	       // a is a signed type and was negative
	       _a < 0 ||

	       // Not invertable means a was truncated during shifting
	       (*_d >> _to_shift) != a))
          })

Quoted 3 lines · Jason Gunthorpe

What about more like this?

          check_shift_overflow(a, s, d) ({

Should that not be: check_shl_overflow() ? Just 'shift' lacks a direction.

Quoted 5 lines

	      // Shift is always performed on the machine's largest unsigned
              u64 _a = a;
	      typeof(s) _s = s;
              typeof(d) _d = d;

Quoted 2 lines

	      // Make s safe against UB
	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;

Should we not do a gcc-plugin or something that fixes that particular UB? Shift acting all retarded like that is just annoying. I feel we should eliminate UBs from the language where possible, like -fno-strict-overflow mandates 2s complement.

Quoted 4 lines

              *_d = (_a << _to_shift);

	       // s is malformed
              (_to_shift != _s ||

Not strictly an overflow though, just not expected behaviour.

Quoted 2 lines

	       // d is a signed type and became negative
	       *_d < 0 ||

Only a problem if it wasn't negative to start out with.

Quoted 2 lines

	       // a is a signed type and was negative
	       _a < 0 ||

Why would that be a problem? You can shift left negative values just fine. The only problem is when you run out of sign bits.

Quoted 3 lines

	       // Not invertable means a was truncated during shifting
	       (*_d >> _to_shift) != a))
          })

And I'm not exactly seeing the use case for this macro. What's the point of a shift-left if you cannot truncate bits. I suppose it's in the name _overflow, but still.

Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Add shift_overflow() helper to help driver authors to ensure that shift operand doesn't cause to overflow, which is very common pattern for RDMA drivers.

Not a huge fan. The other _overflow functions have a different behaviour (in how they return the result and the overflow status) and are type-generic, and I think someone at some point will use such a generically-named helper for stuff other than size_t. At least the array_size and struct_size helpers have size in their name and are specifically about computing the size of something, and are designed to be used directly as arguments to allocators, where SIZE_MAX is a suitable sentinel. I can't see the other patches in this series, so I don't know how you plan on using it, but it should also be usable outside rdma.

Aside: why does b have type size_t?

Does __must_check really make sense for a function without side effects? It doesn't tell gcc to warn if the result is not used in a conditional, it just warns if the result is not used at all, which wouldn't realistically happen for a pure function.

I'd much rather see a type-generic check_shift_overflow (we can agree to leave "left" out of the name) with semantics similar to the other check_*_overflow functions. Then, if a size_t-eating, SIZE_MAX-returning helper is more convenient for rdma, that should be easy to implement on top of that. It shouldn't really be that hard to do. Something like

check_shift_overflow(a, s, d) {
    unsigned _nbits = 8*sizeof(a);
    typeof(a) _a = (a);
    typeof(s) _s = (s);
    typeof(d) _d = (d);

    *_d = ((u64)(_a) << (_s & (_nbits-1)));

    _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s - is_signed_type(a))) !=

0);
}

which should also handle shifts of signed types (though it allows << 0 for negative values; that's easy to also disallow). But the exact semantics should be documented via a bunch of tests (hint hint) exercising corner cases.

Rasmus

From: Leon Romanovsky <[email protected]>

[   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
[   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
[   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
[   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
[   61.188315] Call Trace:
[   61.188661]  dump_stack+0xc7/0x13b
[   61.190427]  ubsan_epilogue+0x9/0x49
[   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
[   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
[   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
[   61.213892]  ib_uverbs_write+0x77e/0xae0
[   61.248018]  vfs_write+0x121/0x3b0
[   61.249831]  ksys_write+0xa1/0x120
[   61.254024]  do_syscall_64+0x7c/0x2a0
[   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   61.259211] RIP: 0033:0x7f54bab70e99
[   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
[   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
[   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
[   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
[   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
[   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000

Cc <[email protected]> # 4.7

Fixes 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")

Cc syzkaller <[email protected]>

Reported-by Noa Osherovich <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/infiniband/hw/mlx5/qp.c +1 -0

index 6034a670859f..8e40263fd40e 100644
+++ b/drivers/infiniband/hw/mlx5/qp.c

Quoted 39 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

[   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
[   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
[   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
[   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
[   61.188315] Call Trace:
[   61.188661]  dump_stack+0xc7/0x13b
[   61.190427]  ubsan_epilogue+0x9/0x49
[   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
[   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
[   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
[   61.213892]  ib_uverbs_write+0x77e/0xae0
[   61.248018]  vfs_write+0x121/0x3b0
[   61.249831]  ksys_write+0xa1/0x120
[   61.254024]  do_syscall_64+0x7c/0x2a0
[   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   61.259211] RIP: 0033:0x7f54bab70e99
[   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
[   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
[   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
[   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
[   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
[   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000

Cc <[email protected]> # 4.7

Fixes 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")

Cc syzkaller <[email protected]>

Reported-by Noa Osherovich <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/infiniband/hw/mlx5/qp.c +1 -0

index 6034a670859f..8e40263fd40e 100644
+++ b/drivers/infiniband/hw/mlx5/qp.c

@@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
 

drivers/infiniband/hw/mlx5/qp.c static int set_user_rq_size(struct mlx5_ib_dev *dev, · line 5381

Quoted 5 lines

drivers/infiniband/hw/mlx5/qp.c +2 -1

 	rwq->wqe_count = ucmd->rq_wqe_count;
 	rwq->wqe_shift = ucmd->rq_wqe_shift;
-	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
+	rwq->buf_size =
+		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);

The casts are redundant, the function argument is already size_t so implicit promotion is guaranteed.

Jason

Quoted 40 lines · Jason Gunthorpe

Quoted 39 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

[   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
[   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
[   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
[   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
[   61.188315] Call Trace:
[   61.188661]  dump_stack+0xc7/0x13b
[   61.190427]  ubsan_epilogue+0x9/0x49
[   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
[   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
[   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
[   61.213892]  ib_uverbs_write+0x77e/0xae0
[   61.248018]  vfs_write+0x121/0x3b0
[   61.249831]  ksys_write+0xa1/0x120
[   61.254024]  do_syscall_64+0x7c/0x2a0
[   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   61.259211] RIP: 0033:0x7f54bab70e99
[   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
[   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
[   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
[   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
[   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
[   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000

Cc <[email protected]> # 4.7

Fixes 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")

Cc syzkaller <[email protected]>

Reported-by Noa Osherovich <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/infiniband/hw/mlx5/qp.c +1 -0

index 6034a670859f..8e40263fd40e 100644
+++ b/drivers/infiniband/hw/mlx5/qp.c

@@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
 

Quoted 8 lines

drivers/infiniband/hw/mlx5/qp.c static int set_user_rq_size(struct mlx5_ib_dev *dev, · line 5381

Quoted 5 lines

drivers/infiniband/hw/mlx5/qp.c +2 -1

 	rwq->wqe_count = ucmd->rq_wqe_count;
 	rwq->wqe_shift = ucmd->rq_wqe_shift;
-	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
+	rwq->buf_size =
+		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);

The casts are redundant, the function argument is already size_t so implicit promotion is guaranteed.

rwq->wqe_count and rwq->wqe_shift are declared as u32 and not as size_t.

https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/hw/mlx5/mlx5_ib.h#L296

From: Leon Romanovsky <[email protected]>

Hi,

This is bunch of patches trigged by running syzkaller internally.

I'm sending them based on rdma-next mainly for two reasons:
1, Most of the patches fix the old issues and it doesn't matter when they will hit the Linus's tree: now or later in a couple of weeks during merge window.
2. They interleave with code cleanup, mlx5-next patches and Michael's feedback on flow counters series.

Thanks

Leon Romanovsky (12):
RDMA/uverbs: Protect from attempts to create flows on unsupported QP RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

RDMA/uverbs: Check existence of create_flow callback RDMA/verbs: Drop kernel variant of create_flow
RDMA/verbs: Drop kernel variant of destroy_flow
net/mlx5: Rate limit errors in command interface RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR RDMA/umem: Don't check for negative return value of dma_map_sg_attrs() RDMA/uverbs: Remove redundant check

overflow.h: Add arithmetic shift helper RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq RDMA/mlx5: Reuse existed shift_overlow helper

Quoted 13 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Hi,

This is bunch of patches trigged by running syzkaller internally.

I'm sending them based on rdma-next mainly for two reasons:
1, Most of the patches fix the old issues and it doesn't matter when they will hit the Linus's tree: now or later in a couple of weeks during merge window.
2. They interleave with code cleanup, mlx5-next patches and Michael's feedback on flow counters series.

Quoted 5 lines

Thanks

Leon Romanovsky (12):
RDMA/uverbs: Protect from attempts to create flows on unsupported QP RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

I applied these two to for-rc

Quoted 7 lines

RDMA/uverbs: Check existence of create_flow callback RDMA/verbs: Drop kernel variant of create_flow
RDMA/verbs: Drop kernel variant of destroy_flow
net/mlx5: Rate limit errors in command interface RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR RDMA/umem: Don't check for negative return value of dma_map_sg_attrs() RDMA/uverbs: Remove redundant check

These to for-next

Quoted 4 lines

overflow.h: Add arithmetic shift helper RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq RDMA/mlx5: Reuse existed shift_overlow helper

And these will have to be respun.

Thanks,
Jason

Quoted 30 lines · Jason Gunthorpe

Quoted 13 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Hi,

This is bunch of patches trigged by running syzkaller internally.

I'm sending them based on rdma-next mainly for two reasons:
1, Most of the patches fix the old issues and it doesn't matter when they will hit the Linus's tree: now or later in a couple of weeks during merge window.
2. They interleave with code cleanup, mlx5-next patches and Michael's feedback on flow counters series.

Quoted 5 lines

Thanks

Leon Romanovsky (12):
RDMA/uverbs: Protect from attempts to create flows on unsupported QP RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

I applied these two to for-rc

Quoted 7 lines

RDMA/uverbs: Check existence of create_flow callback RDMA/verbs: Drop kernel variant of create_flow
RDMA/verbs: Drop kernel variant of destroy_flow
net/mlx5: Rate limit errors in command interface RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR RDMA/umem: Don't check for negative return value of dma_map_sg_attrs() RDMA/uverbs: Remove redundant check

Quoted 1 line

These to for-next

Jason,

We would like to see patch "[PATCH mlx5-next 05/12] net/mlx5:
Rate limit errors in command interface" in out mlx5-next. Is it possible at this point to drop it from for-next, so I'll be able to take it into mlx5-next?

Quoted 35 lines · Leon Romanovsky

Quoted 30 lines · Jason Gunthorpe

Quoted 13 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Hi,

This is bunch of patches trigged by running syzkaller internally.

I'm sending them based on rdma-next mainly for two reasons:
1, Most of the patches fix the old issues and it doesn't matter when they will hit the Linus's tree: now or later in a couple of weeks during merge window.
2. They interleave with code cleanup, mlx5-next patches and Michael's feedback on flow counters series.

Quoted 5 lines

Thanks

Leon Romanovsky (12):
RDMA/uverbs: Protect from attempts to create flows on unsupported QP RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

I applied these two to for-rc

Quoted 7 lines

RDMA/uverbs: Check existence of create_flow callback RDMA/verbs: Drop kernel variant of create_flow
RDMA/verbs: Drop kernel variant of destroy_flow
net/mlx5: Rate limit errors in command interface RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR RDMA/umem: Don't check for negative return value of dma_map_sg_attrs() RDMA/uverbs: Remove redundant check

Quoted 1 line

These to for-next

Jason,

Quoted 4 lines

We would like to see patch "[PATCH mlx5-next 05/12] net/mlx5:
Rate limit errors in command interface" in out mlx5-next. Is it possible at this point to drop it from for-next, so I'll be able to take it into mlx5-next?

Okay, you got to this while it was still 'wip', so it is dropped. Add it to the mlx5-next branch and netdev or rdma can pull it next time there is some reason to pull the branch..

Jason