[PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Leon Romanovsky]

Hi,

There is small set of fixes targeted for rdma-rc.

Thanks

Bodong Wang (1):
  IB/core: Fix ib_wc structure size to remain in 64 bytes boundary

Jack Morgenstein (1):
  IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH
    ports

Leon Romanovsky (1):
  RDMA/mlx5: Fix out-of-bound access while querying AH

Parav Pandit (1):
  RDMA/core: Fix avoid decoding iWarp port as RoCE

 drivers/infiniband/hw/mlx4/main.c       | 13 +++++--------
 drivers/infiniband/hw/mlx5/qp.c         |  7 +++----
 drivers/net/ethernet/mellanox/mlx4/qp.c |  3 +++
 include/rdma/ib_verbs.h                 |  5 ++---
 4 files changed, 13 insertions(+), 15 deletions(-)

--
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 1/4] RDMA/mlx5: Fix out-of-bound access while querying AH

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The rdma_ah_find_type() accesses the port array based on index.

Such call to that function before actually checking the index leads
to the following out-of-bound crash.

==================================================================
BUG: KASAN: slab-out-of-bounds in to_rdma_ah_attr+0xa8/0x3b0
Read of size 4 at addr ffff880019ae2268 by task ibv_rc_pingpong/409

CPU: 0 PID: 409 Comm: ibv_rc_pingpong Not tainted 4.15.0-rc2-00031-gb60a3faf5b83-dirty #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
Call Trace:
 dump_stack+0xe9/0x18f
 print_address_description+0xa2/0x350
 kasan_report+0x3a5/0x400
 to_rdma_ah_attr+0xa8/0x3b0
 mlx5_ib_query_qp+0xd35/0x1330
 ib_query_qp+0x8a/0xb0
 ib_uverbs_query_qp+0x237/0x7f0
 ib_uverbs_write+0x617/0xd80
 __vfs_write+0xf7/0x500
 vfs_write+0x149/0x310
 SyS_write+0xca/0x190
 entry_SYSCALL_64_fastpath+0x18/0x85
RIP: 0033:0x7fe9c7a275a0
RSP: 002b:00007ffee5498738 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fe9c7ce4b00 RCX: 00007fe9c7a275a0
RDX: 0000000000000018 RSI: 00007ffee5498800 RDI: 0000000000000003
RBP: 000055d0c8d3f010 R08: 00007ffee5498800 R09: 0000000000000018
R10: 00000000000000ba R11: 0000000000000246 R12: 0000000000008000
R13: 0000000000004fb0 R14: 000055d0c8d3f050 R15: 00007ffee5498560

Allocated by task 1:
 __kmalloc+0x3f9/0x430
 alloc_mad_private+0x25/0x50
 ib_mad_post_receive_mads+0x204/0xa60
 ib_mad_init_device+0xa59/0x1020
 ib_register_device+0x83a/0xbc0
 mlx5_ib_add+0x50e/0x5c0
 mlx5_add_device+0x142/0x410
 mlx5_register_interface+0x18f/0x210
 mlx5_ib_init+0x56/0x63
 do_one_initcall+0x15b/0x270
 kernel_init_freeable+0x2d8/0x3d0
 kernel_init+0x14/0x190
 ret_from_fork+0x24/0x30

Freed by task 0:
(stack is not available)

The buggy address belongs to the object at ffff880019ae2000
 which belongs to the cache kmalloc-512 of size 512
The buggy address is located 104 bytes to the right of
 512-byte region [ffff880019ae2000, ffff880019ae2200)
The buggy address belongs to the page:
page:000000005d674e18 count:1 mapcount:0 mapping:          (null) index:0x0 compound_mapcount: 0
flags: 0x4000000000008100(slab|head)
raw: 4000000000008100 0000000000000000 0000000000000000 00000001000c000c
raw: dead000000000100 dead000000000200 ffff88001a402000 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff880019ae2100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff880019ae2180: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
>ffff880019ae2200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
                                                          ^
 ffff880019ae2280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff880019ae2300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
Disabling lock debugging due to kernel taint

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/qp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 31ad28853efa..cffe5966aef9 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -4362,12 +4362,11 @@ static void to_rdma_ah_attr(struct mlx5_ib_dev *ibdev,
 
 	memset(ah_attr, 0, sizeof(*ah_attr));
 
-	ah_attr->type = rdma_ah_find_type(&ibdev->ib_dev, path->port);
-	rdma_ah_set_port_num(ah_attr, path->port);
-	if (rdma_ah_get_port_num(ah_attr) == 0 ||
-	    rdma_ah_get_port_num(ah_attr) > MLX5_CAP_GEN(dev, num_ports))
+	if (!path->port || path->port > MLX5_CAP_GEN(dev, num_ports))
 		return;
 
+	ah_attr->type = rdma_ah_find_type(&ibdev->ib_dev, path->port);
+
 	rdma_ah_set_port_num(ah_attr, path->port);
 	rdma_ah_set_sl(ah_attr, path->dci_cfi_prio_sl & 0xf);
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 2/4] IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH ports

[Leon Romanovsky]

From: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

Allocating steerable UD QPs depends on having at least one IB port,
while releasing those QPs does not.

As a result, when there are only ETH ports, the IB (RoCE) driver
requests releasing a qp range whose base qp is zero, with
qp count zero.

When SR-IOV is enabled, and the VF driver is running on a VM over
a hypervisor which treats such qp release calls as errors
(rather than NOPs), we see lines in the VM message log like:

 mlx4_core 0002:00:02.0: Failed to release qp range base:0 cnt:0

Fix this by adding a check for a zero count in mlx4_release_qp_range()
(which thus treats releasing 0 qps as a nop), and eliminating the
check for device managed flow steering when releasing steerable UD QPs.
(Freeing ib_uc_qpns_bitmap unconditionally is also OK, since it
remains NULL when steerable UD QPs are not allocated).

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Fixes: 4196670be786 ("IB/mlx4: Don't allocate range of steerable UD QPs for Ethernet-only device")
Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c       | 13 +++++--------
 drivers/net/ethernet/mellanox/mlx4/qp.c |  3 +++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 8c8a16791a3f..5caf37ba7fff 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2995,9 +2995,8 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 	kfree(ibdev->ib_uc_qpns_bitmap);
 
 err_steer_qp_release:
-	if (ibdev->steering_support == MLX4_STEERING_MODE_DEVICE_MANAGED)
-		mlx4_qp_release_range(dev, ibdev->steer_qpn_base,
-				      ibdev->steer_qpn_count);
+	mlx4_qp_release_range(dev, ibdev->steer_qpn_base,
+			      ibdev->steer_qpn_count);
 err_counter:
 	for (i = 0; i < ibdev->num_ports; ++i)
 		mlx4_ib_delete_counters_table(ibdev, &ibdev->counters_table[i]);
@@ -3102,11 +3101,9 @@ static void mlx4_ib_remove(struct mlx4_dev *dev, void *ibdev_ptr)
 		ibdev->iboe.nb.notifier_call = NULL;
 	}
 
-	if (ibdev->steering_support == MLX4_STEERING_MODE_DEVICE_MANAGED) {
-		mlx4_qp_release_range(dev, ibdev->steer_qpn_base,
-				      ibdev->steer_qpn_count);
-		kfree(ibdev->ib_uc_qpns_bitmap);
-	}
+	mlx4_qp_release_range(dev, ibdev->steer_qpn_base,
+			      ibdev->steer_qpn_count);
+	kfree(ibdev->ib_uc_qpns_bitmap);
 
 	iounmap(ibdev->uar_map);
 	for (p = 0; p < ibdev->num_ports; ++p)
diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c
index 769598f7b6c8..3aaf4bad6c5a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/qp.c
+++ b/drivers/net/ethernet/mellanox/mlx4/qp.c
@@ -287,6 +287,9 @@ void mlx4_qp_release_range(struct mlx4_dev *dev, int base_qpn, int cnt)
 	u64 in_param = 0;
 	int err;
 
+	if (!cnt)
+		return;
+
 	if (mlx4_is_mfunc(dev)) {
 		set_param_l(&in_param, base_qpn);
 		set_param_h(&in_param, cnt);
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 3/4] IB/core: Fix ib_wc structure size to remain in 64 bytes boundary

[Leon Romanovsky]

From: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The change of slid from u16 to u32 results in sizeof(struct ib_wc)
cross 64B boundary, which causes more cache misses. This patch
rearranges the fields and remain the size to 64B.

Pahole output before this change:

struct ib_wc {
        union {
                u64                wr_id;                /*           8 */
                struct ib_cqe *    wr_cqe;               /*           8 */
        };                                               /*     0     8 */
        enum ib_wc_status          status;               /*     8     4 */
        enum ib_wc_opcode          opcode;               /*    12     4 */
        u32                        vendor_err;           /*    16     4 */
        u32                        byte_len;             /*    20     4 */
        struct ib_qp *             qp;                   /*    24     8 */
        union {
                __be32             imm_data;             /*           4 */
                u32                invalidate_rkey;      /*           4 */
        } ex;                                            /*    32     4 */
        u32                        src_qp;               /*    36     4 */
        int                        wc_flags;             /*    40     4 */
        u16                        pkey_index;           /*    44     2 */

        /* XXX 2 bytes hole, try to pack */

        u32                        slid;                 /*    48     4 */
        u8                         sl;                   /*    52     1 */
        u8                         dlid_path_bits;       /*    53     1 */
        u8                         port_num;             /*    54     1 */
        u8                         smac[6];              /*    55     6 */

        /* XXX 1 byte hole, try to pack */

        u16                        vlan_id;              /*    62     2 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        u8                         network_hdr_type;     /*    64     1 */

        /* size: 72, cachelines: 2, members: 17 */
        /* sum members: 62, holes: 2, sum holes: 3 */
        /* padding: 7 */
        /* last cacheline: 8 bytes */
};

Pahole output after this change:

struct ib_wc {
        union {
                u64                wr_id;                /*           8 */
                struct ib_cqe *    wr_cqe;               /*           8 */
        };                                               /*     0     8 */
        enum ib_wc_status          status;               /*     8     4 */
        enum ib_wc_opcode          opcode;               /*    12     4 */
        u32                        vendor_err;           /*    16     4 */
        u32                        byte_len;             /*    20     4 */
        struct ib_qp *             qp;                   /*    24     8 */
        union {
                __be32             imm_data;             /*           4 */
                u32                invalidate_rkey;      /*           4 */
        } ex;                                            /*    32     4 */
        u32                        src_qp;               /*    36     4 */
        u32                        slid;                 /*    40     4 */
        int                        wc_flags;             /*    44     4 */
        u16                        pkey_index;           /*    48     2 */
        u8                         sl;                   /*    50     1 */
        u8                         dlid_path_bits;       /*    51     1 */
        u8                         port_num;             /*    52     1 */
        u8                         smac[6];              /*    53     6 */

        /* XXX 1 byte hole, try to pack */

        u16                        vlan_id;              /*    60     2 */
        u8                         network_hdr_type;     /*    62     1 */

        /* size: 64, cachelines: 1, members: 17 */
        /* sum members: 62, holes: 1, sum holes: 1 */
        /* padding: 1 */
};

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.13
Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")
Signed-off-by: Bodong Wang <bodong-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/rdma/ib_verbs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index fd84cda5ed7c..0d6a110dae7c 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -983,9 +983,9 @@ struct ib_wc {
 		u32		invalidate_rkey;
 	} ex;
 	u32			src_qp;
+	u32			slid;
 	int			wc_flags;
 	u16			pkey_index;
-	u32			slid;
 	u8			sl;
 	u8			dlid_path_bits;
 	u8			port_num;	/* valid only for DR SMPs on switches */
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-rc 4/4] RDMA/core: Fix avoid decoding iWarp port as RoCE

[Leon Romanovsky]

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

rdma_ah_find_type() incorrectly decodes iWARP port as RoCE type,
avoid such incorrect decoding.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/rdma/ib_verbs.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 0d6a110dae7c..20ebf9061962 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3793,8 +3793,7 @@ static inline void rdma_ah_set_grh(struct rdma_ah_attr *attr,
 static inline enum rdma_ah_attr_type rdma_ah_find_type(struct ib_device *dev,
 						       u32 port_num)
 {
-	if ((rdma_protocol_roce(dev, port_num)) ||
-	    (rdma_protocol_iwarp(dev, port_num)))
+	if (rdma_protocol_roce(dev, port_num))
 		return RDMA_AH_ATTR_TYPE_ROCE;
 	else if ((rdma_protocol_ib(dev, port_num)) &&
 		 (rdma_cap_opa_ah(dev, port_num)))
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Chien Tin Tung]

On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> Hi,
> 
> There is small set of fixes targeted for rdma-rc.

For RC?  Are these fixing regressions?  We are already in RC7.

Doug/Jason,

What's your policy on taking fixes late in RC cycle that may/may not be fixing regressions?

Chien

> 
> Thanks
> 
> Bodong Wang (1):
>   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary
> 
> Jack Morgenstein (1):
>   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH
>     ports
> 
> Leon Romanovsky (1):
>   RDMA/mlx5: Fix out-of-bound access while querying AH
> 
> Parav Pandit (1):
>   RDMA/core: Fix avoid decoding iWarp port as RoCE
> 
>  drivers/infiniband/hw/mlx4/main.c       | 13 +++++--------
>  drivers/infiniband/hw/mlx5/qp.c         |  7 +++----
>  drivers/net/ethernet/mellanox/mlx4/qp.c |  3 +++
>  include/rdma/ib_verbs.h                 |  5 ++---
>  4 files changed, 13 insertions(+), 15 deletions(-)
> 
> --
> 2.15.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Fri, Jan 12, 2018 at 08:15:24AM -0600, Chien Tin Tung wrote:
> On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > Hi,
> >
> > There is small set of fixes targeted for rdma-rc.
>
> For RC?  Are these fixing regressions?  We are already in RC7.

Jason was clear last time, he wants to work like Dave, fixes go always
without any relation to -RC.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Chien Tin Tung]

On Fri, Jan 12, 2018 at 05:06:02PM +0200, Leon Romanovsky wrote:
> On Fri, Jan 12, 2018 at 08:15:24AM -0600, Chien Tin Tung wrote:
> > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > > Hi,
> > >
> > > There is small set of fixes targeted for rdma-rc.
> >
> > For RC?  Are these fixing regressions?  We are already in RC7.
> 
> Jason was clear last time, he wants to work like Dave, fixes go always
> without any relation to -RC.

I won't claim to know Dave's process since I don't drectly submit
patches to Netdev.  However given Dave's history with the kernel,
I highly doubt he would accept patches late in RC cycle that would
jeopardize a kernel release.

I would like to hear directly from Doug and Jason on this and so 
we are clear on the policy to be enforced with infiniband subsystem.

In the meantime, I will quote the official documentation from kernel.org
(https://www.kernel.org/doc/html/v4.10/process/howto.html)



4.x kernel tree

[...]

        After two weeks a -rc1 kernel is released and the focus is on making
the new kernel as rock solid as possible. Most of the patches at this point
should fix a regression. Bugs that have always existed are not regressions, so
only push these kinds of fixes if they are important. Please note that a whole
new driver (or filesystem) might be accepted after -rc1 because there is no risk
of causing regressions with such a change as long as the change is self-contained
and does not affect areas outside of the code that is being added. git can be
used to send patches to Linus after -rc1 is released, but the patches need to
also be sent to a public mailing list for review.



Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 09:34:02AM -0600, Chien Tin Tung wrote:
> On Fri, Jan 12, 2018 at 05:06:02PM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 12, 2018 at 08:15:24AM -0600, Chien Tin Tung wrote:
> > > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > > > Hi,
> > > >
> > > > There is small set of fixes targeted for rdma-rc.
> > >
> > > For RC?  Are these fixing regressions?  We are already in RC7.
> > 
> > Jason was clear last time, he wants to work like Dave, fixes go always
> > without any relation to -RC.
> 
> I won't claim to know Dave's process since I don't drectly submit
> patches to Netdev.  However given Dave's history with the kernel,
> I highly doubt he would accept patches late in RC cycle that would
> jeopardize a kernel release.

Well, my definition of 'fixes' is pretty high. For instance fixing a
performance regression is not a 'fix', IMHO.

Fixing a user triggerable out-of-bound oops would be though - as in
these modern times I'm sure someone smart can escalate stuff like that
to a full security compromise of a trusted boot system..

This is why I keep asking for good commit messages for -rc
patches. Explain why you think this deserves to be in -rc, in terms
other people can understand:

 - Can userspace trigger an oops or memory corruption? How?
 - Can the kernel oops or memory corrupt in a actual demonstrated way
   (not theoretical)?
 - Did this actually get hit durring real world testing?
 - Could this escalate to some kind of security issue for a
   trusted-boot kernel?
 
etc.

So, when I've said 'fixes should go to rc', I mean fixes that qualify
as important here:

...  Bugs that have always existed are not regressions, so
     only push these kinds of fixes if they are important.  ...

and I haven't ment 'just anything with a Fixes: tag'.

If I think I can't defend why your patch is 'important' to Linus,
then the likely hood of taking it decreases as we move along the rc
cycle. It is up to the submitter to write a good commit message.

Doug also likes to see patches with a small LOC late in the cycle.

eg if we look at the recent patches:
 - Two security related issues for SRP
 - A locking issue leading to user triggerable memory corruption
   (eg add/remove rxe devices in parallel with netlink queries)
 - In-kernel oops under error situations potentially triggerable
 - Real world oops caused by SE Linux checking, hit during testing
 - A issue that damages our potential future ABI compatability in uverbs
 - User triggerable kernel data corruption due to missing locking

etc..

and some counter examples that went to -next:

- 'RDMA/cma: Fix rdma_cm path querying for RoCE'
  The uAPI has been broken here since v4.12 and nobody noticed
  The use case in user space is very obscure
  Probably would have taken this to -rc around rc1/2/3
- 'net/mlx5: Fix race for multiple RoCE enable'
  This has been broken since v4.13
  Unclear from commit message if this is theoretical, user
  triggerable, or seen in the real world
  Could have been -rc if the risks were identified as real world
- 'IB/{hfi1, qib}: Fix a concurrency issue with device name in
  logging'
  Seems kinda -rc worthy but the LOC is high, the bug has existed
  for soemthing like a decade, and the commit message doesn't
  explain why computing the wrong string is 'important'. Doesn't
  seem to be a oops or security issue.

At the very least this is the best thinking I've come up with after
talking to several people - advice welcome of course.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:

So, my take on the for-rc8-ness, based on the commit messages alone:

> Bodong Wang (1):
>   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary

No, performance optimization only, and not a regression in this cycle

> Jack Morgenstein (1):
>   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH
>     ports

No, only supresses an annoying warning

> Leon Romanovsky (1):
>   RDMA/mlx5: Fix out-of-bound access while querying AH

Yes, fixes a user triggerable out of bounds access

> Parav Pandit (1):
>   RDMA/core: Fix avoid decoding iWarp port as RoCE

Unknown, commit message is too short. Parav, please explain more why this
this is worthy of 'CC: stable' ?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Fri, Jan 12, 2018 at 10:02:28AM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
>
> So, my take on the for-rc8-ness, based on the commit messages alone:
>
> > Bodong Wang (1):
> >   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary
>
> No, performance optimization only, and not a regression in this cycle
>

Sorry, but it is fix to performance regression, and not optimization.

> > Jack Morgenstein (1):
> >   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH
> >     ports
>
> No, only supresses an annoying warning
>
> > Leon Romanovsky (1):
> >   RDMA/mlx5: Fix out-of-bound access while querying AH
>
> Yes, fixes a user triggerable out of bounds access

Right

>
> > Parav Pandit (1):
> >   RDMA/core: Fix avoid decoding iWarp port as RoCE
>
> Unknown, commit message is too short. Parav, please explain more why this
> this is worthy of 'CC: stable' ?

Parav has nothing to do with "CC: stable@", I'm the person who added it.

The rationale behind it that it so basic change so it worth to have in
stable. Also, it goes as all our countless fixes to those two series:
Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")

Thanks

> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 08:39:58PM +0200, Leon Romanovsky wrote:
> On Fri, Jan 12, 2018 at 10:02:28AM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> >
> > So, my take on the for-rc8-ness, based on the commit messages alone:
> >
> > > Bodong Wang (1):
> > >   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary
> >
> > No, performance optimization only, and not a regression in this cycle
> >
> 
> Sorry, but it is fix to performance regression, and not optimization.

regression across multiple kernel cycles and not a regression in this
cycle. So the performance optimization needs to be justified to raise
to 'important'. There is no performance data in the commit message, so
I can't judge it as OK for for-rc

> > > Parav Pandit (1):
> > >   RDMA/core: Fix avoid decoding iWarp port as RoCE
> >
> > Unknown, commit message is too short. Parav, please explain more why this
> > this is worthy of 'CC: stable' ?
> 
> Parav has nothing to do with "CC: stable@", I'm the person who added it.
> 
> The rationale behind it that it so basic change so it worth to have in
> stable. Also, it goes as all our countless fixes to those two series:
> Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
> Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")

Nevertheless, the commit message does not explain to me or Linus why
this is *important* - it does not talk about what user visible
consequnce there is to this bug.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Sat, Jan 13, 2018 at 12:18:06PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 12, 2018 at 08:39:58PM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 12, 2018 at 10:02:28AM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > >
> > > So, my take on the for-rc8-ness, based on the commit messages alone:
> > >
> > > > Bodong Wang (1):
> > > >   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary
> > >
> > > No, performance optimization only, and not a regression in this cycle
> > >
> >
> > Sorry, but it is fix to performance regression, and not optimization.
>
> regression across multiple kernel cycles and not a regression in this
> cycle. So the performance optimization needs to be justified to raise
> to 'important'. There is no performance data in the commit message, so
> I can't judge it as OK for for-rc
>
> > > > Parav Pandit (1):
> > > >   RDMA/core: Fix avoid decoding iWarp port as RoCE
> > >
> > > Unknown, commit message is too short. Parav, please explain more why this
> > > this is worthy of 'CC: stable' ?
> >
> > Parav has nothing to do with "CC: stable@", I'm the person who added it.
> >
> > The rationale behind it that it so basic change so it worth to have in
> > stable. Also, it goes as all our countless fixes to those two series:
> > Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
> > Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")
>
> Nevertheless, the commit message does not explain to me or Linus why
> this is *important* - it does not talk about what user visible
> consequnce there is to this bug.

I'm a little bit confused here, when I submitted Fixes to -next, you
asked from em to send such patches to -rc and now you are asking to send
Fixes to -next.

It will be better if you and Doug have more clear definition for -rc
than LOC count and gut feelings.

For example, Dave took this series for -rc7/8, because it fixes the code
and don't add new features.
https://www.spinics.net/lists/netdev/msg477875.html

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Sat, Jan 13, 2018 at 10:11:18PM +0200, Leon Romanovsky wrote:

> > > The rationale behind it that it so basic change so it worth to have in
> > > stable. Also, it goes as all our countless fixes to those two series:
> > > Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
> > > Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")
> >
> > Nevertheless, the commit message does not explain to me or Linus why
> > this is *important* - it does not talk about what user visible
> > consequnce there is to this bug.
> 
> I'm a little bit confused here, when I submitted Fixes to -next, you
> asked from em to send such patches to -rc and now you are asking to send
> Fixes to -next.

I never ment that every patch with a Fixes: should go to -rc, I mean
that 'fixes' (lower case) suitable for -rc should go to -rc and not
-next.

And I've always ment that patches going to -rc need to have clear
commit messages that explain to Linus why they are worthy of -rc.

This is to help the overall broader community to ensure that patches
we deem important are made more visible and not just dumped into
for-next.

It is not just Linus asking for this. The distros *need* quality
commit messages to manage their own backporting activities.

> It will be better if you and Doug have more clear definition for -rc
> than LOC count and gut feelings.

Well, show me a guideline from Linus and we can follow it..  Linus has
some standard that is sort of deliberaly nebulous and we have to try
to follow it. I've already outlined my detailed thinking in a recent
email.

Linus is clearly concerned about the risk of introducing regressions
as the rc cycle moves along, so the commit message needs to explain
the benifit of the fix as worth taking a regression risk.

So again, back to 'RDMA/core: Fix avoid decoding iWarp port as
RoCE'. Looking at the actual patch now.. To a casual reader
it changes iwarp's RDMA_AH_ATTR_TYPE_ROCE to RDMA_AH_ATTR_TYPE_IB.

Why don't we have a RDMA_AH_ATTR_TYPE_IWARP? Apparently because iWarp
doesn't use struct rdma_ah_attr at all!

So the proposed patch apparently *does nothing* - that is clearly not
acceptable to for-rc at any point, and it certainly isn't worthy of
-stable. (so unless I hear a better explanation the -stable tag will
be dropped too)

Maybe I'm wrong, but the commit message *DOES NOT EXPLAIN*.

> For example, Dave took this series for -rc7/8, because it fixes the code
> and don't add new features.
> https://www.spinics.net/lists/netdev/msg477875.html

And Linus gives netdev more latitude than he gives rdma, so you cannot
use what DaveM accepts as a guideline for what we should accept. In
practice that means rdma gets more selective as the rc cycle moves on
and maybe netdev doesn't.

For instance, I think Linus would be angry with RDMA if we sent
something like 'net/mlx5e: Add error print in ETS init' to -rc8.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]

On Sat, Jan 13, 2018 at 01:46:09PM -0700, Jason Gunthorpe wrote:
> On Sat, Jan 13, 2018 at 10:11:18PM +0200, Leon Romanovsky wrote:
>
> > > > The rationale behind it that it so basic change so it worth to have in
> > > > stable. Also, it goes as all our countless fixes to those two series:
> > > > Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
> > > > Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")
> > >
> > > Nevertheless, the commit message does not explain to me or Linus why
> > > this is *important* - it does not talk about what user visible
> > > consequnce there is to this bug.
> >
> > I'm a little bit confused here, when I submitted Fixes to -next, you
> > asked from em to send such patches to -rc and now you are asking to send
> > Fixes to -next.
>
> I never ment that every patch with a Fixes: should go to -rc, I mean
> that 'fixes' (lower case) suitable for -rc should go to -rc and not
> -next.
>
> And I've always ment that patches going to -rc need to have clear
> commit messages that explain to Linus why they are worthy of -rc.
>
> This is to help the overall broader community to ensure that patches
> we deem important are made more visible and not just dumped into
> for-next.
>
> It is not just Linus asking for this. The distros *need* quality
> commit messages to manage their own backporting activities.
>
> > It will be better if you and Doug have more clear definition for -rc
> > than LOC count and gut feelings.
>
> Well, show me a guideline from Linus and we can follow it..  Linus has
> some standard that is sort of deliberaly nebulous and we have to try
> to follow it. I've already outlined my detailed thinking in a recent
> email.
>
> Linus is clearly concerned about the risk of introducing regressions
> as the rc cycle moves along, so the commit message needs to explain
> the benifit of the fix as worth taking a regression risk.
>
> So again, back to 'RDMA/core: Fix avoid decoding iWarp port as
> RoCE'. Looking at the actual patch now.. To a casual reader
> it changes iwarp's RDMA_AH_ATTR_TYPE_ROCE to RDMA_AH_ATTR_TYPE_IB.
>
> Why don't we have a RDMA_AH_ATTR_TYPE_IWARP? Apparently because iWarp
> doesn't use struct rdma_ah_attr at all!
>
> So the proposed patch apparently *does nothing* - that is clearly not
> acceptable to for-rc at any point, and it certainly isn't worthy of
> -stable. (so unless I hear a better explanation the -stable tag will
> be dropped too)
>
> Maybe I'm wrong, but the commit message *DOES NOT EXPLAIN*.
>
> > For example, Dave took this series for -rc7/8, because it fixes the code
> > and don't add new features.
> > https://www.spinics.net/lists/netdev/msg477875.html
>
> And Linus gives netdev more latitude than he gives rdma, so you cannot
> use what DaveM accepts as a guideline for what we should accept. In
> practice that means rdma gets more selective as the rc cycle moves on
> and maybe netdev doesn't.
>
> For instance, I think Linus would be angry with RDMA if we sent
> something like 'net/mlx5e: Add error print in ETS init' to -rc8.


Ok, let's close this discussion.

I learnt from it three things:

1. I'll submit all my fixes to -next, and you are free to decide by
yourself how to sort them. I don't want to waste my time on trying to juggle
patches between various branches and do postdoc on every patch, in
order to understand how Linus will feel about it. Currently, my decision is
really simple - bug/not bug, and I don't see any reason to complicate it.

2. This kernel release will be DOA for me without patch
"RDMA/mlx5: Fix out-of-bound access while querying AH".

3. -stable tag is a perfect example of cargo cult in RDMA community.
There are number of ways how patches are landed in stable trees
and stable@... just expedite things [1].

Thanks

[1] https://lwn.net/Articles/701304/

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[jackm]

On Fri, 12 Jan 2018 10:02:28 -0700
Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:

> On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> 
> So, my take on the for-rc8-ness, based on the commit messages alone:
> 
> > Bodong Wang (1):
> >   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary  
> 
> No, performance optimization only, and not a regression in this cycle
> 
> > Jack Morgenstein (1):
> >   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have
> > only ETH ports  
> 
> No, only supresses an annoying warning

It does not "suppress" the warning, it fixes the bug which caused
the warning to be written in the log.

The warning happens BECAUSE of the incorrect release attempt.
When the bug in the code is fixed, the warning does not occur.
It is true that at present the annoying (and worrisome) warning in the
log is the only problem resulting from this bug -- but it is still
a bug, and needs to be fixed.

> 
> > Leon Romanovsky (1):
> >   RDMA/mlx5: Fix out-of-bound access while querying AH  
> 
> Yes, fixes a user triggerable out of bounds access
> 
> > Parav Pandit (1):
> >   RDMA/core: Fix avoid decoding iWarp port as RoCE  
> 
> Unknown, commit message is too short. Parav, please explain more why
> this this is worthy of 'CC: stable' ?
> 
> Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Sun, Jan 14, 2018 at 07:15:02PM +0200, jackm wrote:
> On Fri, 12 Jan 2018 10:02:28 -0700
> Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> 
> > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > 
> > So, my take on the for-rc8-ness, based on the commit messages alone:

> > > Jack Morgenstein (1):
> > >   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have
> > > only ETH ports  
> > 
> > No, only supresses an annoying warning
> 
> It does not "suppress" the warning, it fixes the bug which caused
> the warning to be written in the log.
> 
> It is true that at present the annoying (and worrisome) warning in the
> log is the only problem resulting from this bug

Well, that is what I was trying to say.

> -- but it is still a bug, and needs to be fixed.

And it will be fixed, the only question here is if it should be fixed
in 4.15 or 4.16

I think Linus would yell at Doug and I if we sent a patch whose only
outcome was to remove a warning at this point in the cycle. Does anyone
know differently?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 1/4] RDMA/mlx5: Fix out-of-bound access while querying AH

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 07:58:39AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The rdma_ah_find_type() accesses the port array based on index.
> 
> Such call to that function before actually checking the index leads
> to the following out-of-bound crash.
> 
> Disabling lock debugging due to kernel taint
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/hw/mlx5/qp.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Applied to for-rc, I revised the commit message to draw attention that
this can be triggered from userspace.

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Jason Gunthorpe]

On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:

> Leon Romanovsky (1):
>   RDMA/mlx5: Fix out-of-bound access while querying AH

This one when to for-rc and it causes a merge conflict with for-next
we will have to handle

> Bodong Wang (1):
>   IB/core: Fix ib_wc structure size to remain in 64 bytes boundary

> Jack Morgenstein (1):
>   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have only ETH
>     ports
> 
> Parav Pandit (1):
>   RDMA/core: Fix avoid decoding iWarp port as RoCE

These three went to for-next, I rewrote the commit message for the
last one

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[jackm]

On Sun, 14 Jan 2018 11:22:56 -0700
Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:

> On Sun, Jan 14, 2018 at 07:15:02PM +0200, jackm wrote:
> > On Fri, 12 Jan 2018 10:02:28 -0700
> > Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> >   
> > > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > > 
> > > So, my take on the for-rc8-ness, based on the commit messages
> > > alone:  
> 
> > > > Jack Morgenstein (1):
> > > >   IB/mlx4: Fix incorrectly releasing steerable UD QPs when have
> > > > only ETH ports    
> > > 
> > > No, only supresses an annoying warning  
> > 
> > It does not "suppress" the warning, it fixes the bug which caused
> > the warning to be written in the log.
> > 
> > It is true that at present the annoying (and worrisome) warning in
> > the log is the only problem resulting from this bug  
> 
> Well, that is what I was trying to say.
> 
> > -- but it is still a bug, and needs to be fixed.  
> 
> And it will be fixed, the only question here is if it should be fixed
> in 4.15 or 4.16

Agree for submitting to 4.16 -- this is not a critical fix.
> 
> I think Linus would yell at Doug and I if we sent a patch whose only
> outcome was to remove a warning at this point in the cycle. Does
> anyone know differently?
> 
> Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 4759 bytes --]

On Fri, 2018-01-12 at 09:58 -0700, Jason Gunthorpe wrote:
> On Fri, Jan 12, 2018 at 09:34:02AM -0600, Chien Tin Tung wrote:
> > On Fri, Jan 12, 2018 at 05:06:02PM +0200, Leon Romanovsky wrote:
> > > On Fri, Jan 12, 2018 at 08:15:24AM -0600, Chien Tin Tung wrote:
> > > > On Fri, Jan 12, 2018 at 07:58:38AM +0200, Leon Romanovsky wrote:
> > > > > Hi,
> > > > > 
> > > > > There is small set of fixes targeted for rdma-rc.
> > > > 
> > > > For RC?  Are these fixing regressions?  We are already in RC7.
> > > 
> > > Jason was clear last time, he wants to work like Dave, fixes go always
> > > without any relation to -RC.
> > 
> > I won't claim to know Dave's process since I don't drectly submit
> > patches to Netdev.  However given Dave's history with the kernel,
> > I highly doubt he would accept patches late in RC cycle that would
> > jeopardize a kernel release.
> 
> Well, my definition of 'fixes' is pretty high. For instance fixing a
> performance regression is not a 'fix', IMHO.
> 
> Fixing a user triggerable out-of-bound oops would be though - as in
> these modern times I'm sure someone smart can escalate stuff like that
> to a full security compromise of a trusted boot system..
> 
> This is why I keep asking for good commit messages for -rc
> patches. Explain why you think this deserves to be in -rc, in terms
> other people can understand:
> 
>  - Can userspace trigger an oops or memory corruption? How?
>  - Can the kernel oops or memory corrupt in a actual demonstrated way
>    (not theoretical)?
>  - Did this actually get hit durring real world testing?
>  - Could this escalate to some kind of security issue for a
>    trusted-boot kernel?
>  
> etc.
> 
> So, when I've said 'fixes should go to rc', I mean fixes that qualify
> as important here:
> 
> ...  Bugs that have always existed are not regressions, so
>      only push these kinds of fixes if they are important.  ...
> 
> and I haven't ment 'just anything with a Fixes: tag'.
> 
> If I think I can't defend why your patch is 'important' to Linus,
> then the likely hood of taking it decreases as we move along the rc
> cycle. It is up to the submitter to write a good commit message.
> 
> Doug also likes to see patches with a small LOC late in the cycle.

Indeed.  The larger the line count, the harder it is to guarantee that
you aren't just introducing a new bug.  At some point, the devil you
know is better than the devil you don't.  So, if the bug in question is
an oops, or a locking issue resulting in deadlock, or a use after free
that can result in nothing all the way up to silent data corruption or
oops, then those obviously are the ones that I'm going to take on up
into late rc cycles, and even if their LOC count is higher than I like,
if testing shows it solves the problem, I'll take it.

For lesser issues, I'm more likely to take it late if the problem is
obvious and the fix is small so that we can have a high degree of
certainty that the fix won't introduce any other problems.

> eg if we look at the recent patches:
>  - Two security related issues for SRP
>  - A locking issue leading to user triggerable memory corruption
>    (eg add/remove rxe devices in parallel with netlink queries)
>  - In-kernel oops under error situations potentially triggerable
>  - Real world oops caused by SE Linux checking, hit during testing
>  - A issue that damages our potential future ABI compatability in uverbs
>  - User triggerable kernel data corruption due to missing locking
> 
> etc..
> 
> and some counter examples that went to -next:
> 
> - 'RDMA/cma: Fix rdma_cm path querying for RoCE'
>   The uAPI has been broken here since v4.12 and nobody noticed
>   The use case in user space is very obscure
>   Probably would have taken this to -rc around rc1/2/3
> - 'net/mlx5: Fix race for multiple RoCE enable'
>   This has been broken since v4.13
>   Unclear from commit message if this is theoretical, user
>   triggerable, or seen in the real world
>   Could have been -rc if the risks were identified as real world
> - 'IB/{hfi1, qib}: Fix a concurrency issue with device name in
>   logging'
>   Seems kinda -rc worthy but the LOC is high, the bug has existed
>   for soemthing like a decade, and the commit message doesn't
>   explain why computing the wrong string is 'important'. Doesn't
>   seem to be a oops or security issue.
> 
> At the very least this is the best thinking I've come up with after
> talking to several people - advice welcome of course.
> 
> Jason

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-rc 0/4] RDMA mlx4/mlx5 fixes

[Don Dutile]

On 01/13/2018 03:46 PM, Jason Gunthorpe wrote:
> On Sat, Jan 13, 2018 at 10:11:18PM +0200, Leon Romanovsky wrote:
>
>>>> The rationale behind it that it so basic change so it worth to have in
>>>> stable. Also, it goes as all our countless fixes to those two series:
>>>> Fixes: 44c58487d51a ("IB/core: Define 'ib' and 'roce' rdma_ah_attr types")
>>>> Fixes: 7db20ecd1d97 ("IB/core: Change wc.slid from 16 to 32 bits")
>>>
>>> Nevertheless, the commit message does not explain to me or Linus why
>>> this is *important* - it does not talk about what user visible
>>> consequnce there is to this bug.
>>
>> I'm a little bit confused here, when I submitted Fixes to -next, you
>> asked from em to send such patches to -rc and now you are asking to send
>> Fixes to -next.
>
> I never ment that every patch with a Fixes: should go to -rc, I mean
> that 'fixes' (lower case) suitable for -rc should go to -rc and not
> -next.
>
> And I've always ment that patches going to -rc need to have clear
> commit messages that explain to Linus why they are worthy of -rc.
>
> This is to help the overall broader community to ensure that patches
> we deem important are made more visible and not just dumped into
> for-next.
>
> It is not just Linus asking for this. The distros *need* quality
> commit messages to manage their own backporting activities.
>
Here, here!
As a distro (rdma) maintainer, I can unequivocally say that commit messages
related to bug fixes need improvement.
All too often we see "fixes a bug in x-y-z" but doesn't give a use-case,
or worse, a test to show the bug, so a verification can be done -- esp. for
distros to consume/use/verify their backport -- or even the need for the backport.
Unless it's a race condition, most failures are easily described, and
a full-blown test is not needed.  But no statement as to how to replicate a
bug is being lazy, IMO.

Note, that distro maintainer's most often have release criteria that
they have to meet to allow them to backport a supposed bug-fix, b/c the
release team looks at every addition as an opportunity for a new bug.
So, upstream commits that have clearly documented use-cases, tests/verifications
would go a long way to speed up the number and tie it takes to backport each
commit.
On the flip side, the criteria for bug fix inclusion is quite different for a distro
then an upstream -rc as well.
e.g., a distro will take a trivial warning/error/info bug fix to minimize unnecessary support
calls, while upstream is content to push such a fix to -next, and not -rc.
So _every_ bug fix is important to a distro, and thus, every (bug fix) commit log
is important to have as much info so a release-criteria decision can be made, or more often,
justified by distro maintainers.

So, my suggestion:
Write your bug fixes as if they had to meet a distro's release scrutiny.
I'll bet if you did that, you'd find inclusion/exclusion in -rc & -next to
be trivial.
I'm sure if a number of recent new functional changes were written to
have better testing information, holes in it, and the resulting bugs from them,
would be far more obvious as well.

- Don

>> It will be better if you and Doug have more clear definition for -rc
>> than LOC count and gut feelings.
>
> Well, show me a guideline from Linus and we can follow it..  Linus has
> some standard that is sort of deliberaly nebulous and we have to try
> to follow it. I've already outlined my detailed thinking in a recent
> email.
>
> Linus is clearly concerned about the risk of introducing regressions
> as the rc cycle moves along, so the commit message needs to explain
> the benifit of the fix as worth taking a regression risk.
>
> So again, back to 'RDMA/core: Fix avoid decoding iWarp port as
> RoCE'. Looking at the actual patch now.. To a casual reader
> it changes iwarp's RDMA_AH_ATTR_TYPE_ROCE to RDMA_AH_ATTR_TYPE_IB.
>
> Why don't we have a RDMA_AH_ATTR_TYPE_IWARP? Apparently because iWarp
> doesn't use struct rdma_ah_attr at all!
>
> So the proposed patch apparently *does nothing* - that is clearly not
> acceptable to for-rc at any point, and it certainly isn't worthy of
> -stable. (so unless I hear a better explanation the -stable tag will
> be dropped too)
>
> Maybe I'm wrong, but the commit message *DOES NOT EXPLAIN*.
>
>> For example, Dave took this series for -rc7/8, because it fixes the code
>> and don't add new features.
>> https://www.spinics.net/lists/netdev/msg477875.html
>
> And Linus gives netdev more latitude than he gives rdma, so you cannot
> use what DaveM accepts as a guideline for what we should accept. In
> practice that means rdma gets more selective as the rc cycle moves on
> and maybe netdev doesn't.
>
> For instance, I think Linus would be angry with RDMA if we sent
> something like 'net/mlx5e: Add error print in ETS init' to -rc8.
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html