* [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
@ 2017-11-27 11:25 Leon Romanovsky
2017-11-27 22:03 ` Don Dutile
0 siblings, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2017-11-27 11:25 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Daniel Jurgens, Paul Moore, Don Dutile, stable,
Leon Romanovsky
From: Daniel Jurgens <danielj@mellanox.com>
For now the only LSM security enforcement mechanism available is
specific to InfiniBand. Bypass enforcement for non-IB link types.
This fixes a regression where modify_qp fails for iWARP because
querying the PKEY returns -EINVAL.
Cc: Paul Moore <paul@paul-moore.com>
Cc: Don Dutile <ddutile@redhat.com>
Cc: stable@vger.kernel.org
Reported-by: Potnuri Bharat Teja <bharat@chelsio.com>
Fixes: d291f1a65232("IB/core: Enforce PKey security on QPs")
Fixes: 47a2b338fe63("IB/core: Enforce security on management datagrams")
Signed-off-by: Daniel Jurgens <danielj@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Tested-by: Potnuri Bharat Teja <bharat@chelsio.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
Changelog:
v1->v2: Fixed build errors
v0->v1: Added proper SElinux patch
---
drivers/infiniband/core/security.c | 43 ++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
index 23278ed5be45..06c608c07b65 100644
--- a/drivers/infiniband/core/security.c
+++ b/drivers/infiniband/core/security.c
@@ -417,8 +417,17 @@ void ib_close_shared_qp_security(struct ib_qp_security *sec)
int ib_create_qp_security(struct ib_qp *qp, struct ib_device *dev)
{
+ u8 i = rdma_start_port(dev);
+ bool is_ib = false;
int ret;
+ while (i <= rdma_end_port(dev) && !is_ib)
+ is_ib = rdma_protocol_ib(dev, i++);
+
+ /* If this isn't an IB device don't create the security context */
+ if (!is_ib)
+ return 0;
+
qp->qp_sec = kzalloc(sizeof(*qp->qp_sec), GFP_KERNEL);
if (!qp->qp_sec)
return -ENOMEM;
@@ -441,6 +450,10 @@ EXPORT_SYMBOL(ib_create_qp_security);
void ib_destroy_qp_security_begin(struct ib_qp_security *sec)
{
+ /* Return if not IB */
+ if (!sec)
+ return;
+
mutex_lock(&sec->mutex);
/* Remove the QP from the lists so it won't get added to
@@ -470,6 +483,10 @@ void ib_destroy_qp_security_abort(struct ib_qp_security *sec)
int ret;
int i;
+ /* Return if not IB */
+ if (!sec)
+ return;
+
/* If a concurrent cache update is in progress this
* QP security could be marked for an error state
* transition. Wait for this to complete.
@@ -505,6 +522,10 @@ void ib_destroy_qp_security_end(struct ib_qp_security *sec)
{
int i;
+ /* Return if not IB */
+ if (!sec)
+ return;
+
/* If a concurrent cache update is occurring we must
* wait until this QP security structure is processed
* in the QP to error flow before destroying it because
@@ -565,13 +586,19 @@ int ib_security_modify_qp(struct ib_qp *qp,
bool pps_change = ((qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) ||
(qp_attr_mask & IB_QP_ALT_PATH));
+ WARN_ONCE((qp_attr_mask & IB_QP_PORT &&
+ rdma_protocol_ib(real_qp->device, qp_attr->port_num) &&
+ !real_qp->qp_sec),
+ "%s: QP security is not initialized for IB QP: %d\n",
+ __func__, real_qp->qp_num);
+
/* The port/pkey settings are maintained only for the real QP. Open
* handles on the real QP will be in the shared_qp_list. When
* enforcing security on the real QP all the shared QPs will be
* checked as well.
*/
- if (pps_change && !special_qp) {
+ if (pps_change && !special_qp && real_qp->qp_sec) {
mutex_lock(&real_qp->qp_sec->mutex);
new_pps = get_new_pps(real_qp,
qp_attr,
@@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
qp_attr_mask,
udata);
- if (pps_change && !special_qp) {
+ if (pps_change && !special_qp && real_qp->qp_sec) {
/* Clean up the lists and free the appropriate
* ports_pkeys structure.
*/
@@ -631,6 +658,9 @@ int ib_security_pkey_access(struct ib_device *dev,
u16 pkey;
int ret;
+ if (!rdma_protocol_ib(dev, port_num))
+ return 0;
+
ret = ib_get_cached_pkey(dev, port_num, pkey_index, &pkey);
if (ret)
return ret;
@@ -665,6 +695,9 @@ int ib_mad_agent_security_setup(struct ib_mad_agent *agent,
{
int ret;
+ if (!rdma_protocol_ib(agent->device, agent->port_num))
+ return 0;
+
ret = security_ib_alloc_security(&agent->security);
if (ret)
return ret;
@@ -690,6 +723,9 @@ int ib_mad_agent_security_setup(struct ib_mad_agent *agent,
void ib_mad_agent_security_cleanup(struct ib_mad_agent *agent)
{
+ if (!rdma_protocol_ib(agent->device, agent->port_num))
+ return;
+
security_ib_free_security(agent->security);
if (agent->lsm_nb_reg)
unregister_lsm_notifier(&agent->lsm_nb);
@@ -697,6 +733,9 @@ void ib_mad_agent_security_cleanup(struct ib_mad_agent *agent)
int ib_mad_enforce_security(struct ib_mad_agent_private *map, u16 pkey_index)
{
+ if (!rdma_protocol_ib(map->agent.device, map->agent.port_num))
+ return 0;
+
if (map->agent.qp->qp_type == IB_QPT_SMI && !map->agent.smp_allowed)
return -EACCES;
--
2.15.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-27 11:25 [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand Leon Romanovsky
@ 2017-11-27 22:03 ` Don Dutile
2017-11-27 22:58 ` Daniel Jurgens
0 siblings, 1 reply; 7+ messages in thread
From: Don Dutile @ 2017-11-27 22:03 UTC (permalink / raw)
To: Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Daniel Jurgens, Paul Moore, stable
On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
> From: Daniel Jurgens <danielj@mellanox.com>
>
> For now the only LSM security enforcement mechanism available is
> specific to InfiniBand. Bypass enforcement for non-IB link types.
> This fixes a regression where modify_qp fails for iWARP because
> querying the PKEY returns -EINVAL.
>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Don Dutile <ddutile@redhat.com>
> Cc: stable@vger.kernel.org
> Reported-by: Potnuri Bharat Teja <bharat@chelsio.com>
> Fixes: d291f1a65232("IB/core: Enforce PKey security on QPs")
> Fixes: 47a2b338fe63("IB/core: Enforce security on management datagrams")
> Signed-off-by: Daniel Jurgens <danielj@mellanox.com>
> Reviewed-by: Parav Pandit <parav@mellanox.com>
> Tested-by: Potnuri Bharat Teja <bharat@chelsio.com>
> Signed-off-by: Leon Romanovsky <leon@kernel.org>
> ---
> Changelog:
> v1->v2: Fixed build errors
> v0->v1: Added proper SElinux patch
> ---
> drivers/infiniband/core/security.c | 43 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
> index 23278ed5be45..06c608c07b65 100644
> --- a/drivers/infiniband/core/security.c
> +++ b/drivers/infiniband/core/security.c
> @@ -417,8 +417,17 @@ void ib_close_shared_qp_security(struct ib_qp_security *sec)
>
> int ib_create_qp_security(struct ib_qp *qp, struct ib_device *dev)
> {
> + u8 i = rdma_start_port(dev);
> + bool is_ib = false;
> int ret;
>
> + while (i <= rdma_end_port(dev) && !is_ib)
> + is_ib = rdma_protocol_ib(dev, i++);
> +
> + /* If this isn't an IB device don't create the security context */
> + if (!is_ib)
> + return 0;
> +
> qp->qp_sec = kzalloc(sizeof(*qp->qp_sec), GFP_KERNEL);
> if (!qp->qp_sec)
> return -ENOMEM;
> @@ -441,6 +450,10 @@ EXPORT_SYMBOL(ib_create_qp_security);
>
> void ib_destroy_qp_security_begin(struct ib_qp_security *sec)
> {
> + /* Return if not IB */
> + if (!sec)
> + return;
> +
> mutex_lock(&sec->mutex);
>
> /* Remove the QP from the lists so it won't get added to
> @@ -470,6 +483,10 @@ void ib_destroy_qp_security_abort(struct ib_qp_security *sec)
> int ret;
> int i;
>
> + /* Return if not IB */
> + if (!sec)
> + return;
> +
> /* If a concurrent cache update is in progress this
> * QP security could be marked for an error state
> * transition. Wait for this to complete.
> @@ -505,6 +522,10 @@ void ib_destroy_qp_security_end(struct ib_qp_security *sec)
> {
> int i;
>
> + /* Return if not IB */
> + if (!sec)
> + return;
> +
> /* If a concurrent cache update is occurring we must
> * wait until this QP security structure is processed
> * in the QP to error flow before destroying it because
> @@ -565,13 +586,19 @@ int ib_security_modify_qp(struct ib_qp *qp,
> bool pps_change = ((qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) ||
> (qp_attr_mask & IB_QP_ALT_PATH));
>
> + WARN_ONCE((qp_attr_mask & IB_QP_PORT &&
> + rdma_protocol_ib(real_qp->device, qp_attr->port_num) &&
> + !real_qp->qp_sec),
> + "%s: QP security is not initialized for IB QP: %d\n",
> + __func__, real_qp->qp_num);
> +
> /* The port/pkey settings are maintained only for the real QP. Open
> * handles on the real QP will be in the shared_qp_list. When
> * enforcing security on the real QP all the shared QPs will be
> * checked as well.
> */
>
> - if (pps_change && !special_qp) {
> + if (pps_change && !special_qp && real_qp->qp_sec) {
> mutex_lock(&real_qp->qp_sec->mutex);
> new_pps = get_new_pps(real_qp,
> qp_attr,
> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
> qp_attr_mask,
> udata);
>
> - if (pps_change && !special_qp) {
> + if (pps_change && !special_qp && real_qp->qp_sec) {
> /* Clean up the lists and free the appropriate
> * ports_pkeys structure.
> */
> @@ -631,6 +658,9 @@ int ib_security_pkey_access(struct ib_device *dev,
> u16 pkey;
> int ret;
>
> + if (!rdma_protocol_ib(dev, port_num))
> + return 0;
> +
> ret = ib_get_cached_pkey(dev, port_num, pkey_index, &pkey);
> if (ret)
> return ret;
> @@ -665,6 +695,9 @@ int ib_mad_agent_security_setup(struct ib_mad_agent *agent,
> {
> int ret;
>
> + if (!rdma_protocol_ib(agent->device, agent->port_num))
> + return 0;
> +
> ret = security_ib_alloc_security(&agent->security);
> if (ret)
> return ret;
> @@ -690,6 +723,9 @@ int ib_mad_agent_security_setup(struct ib_mad_agent *agent,
>
> void ib_mad_agent_security_cleanup(struct ib_mad_agent *agent)
> {
> + if (!rdma_protocol_ib(agent->device, agent->port_num))
> + return;
> +
> security_ib_free_security(agent->security);
> if (agent->lsm_nb_reg)
> unregister_lsm_notifier(&agent->lsm_nb);
> @@ -697,6 +733,9 @@ void ib_mad_agent_security_cleanup(struct ib_mad_agent *agent)
>
> int ib_mad_enforce_security(struct ib_mad_agent_private *map, u16 pkey_index)
> {
> + if (!rdma_protocol_ib(map->agent.device, map->agent.port_num))
> + return 0;
> +
> if (map->agent.qp->qp_type == IB_QPT_SMI && !map->agent.smp_allowed)
> return -EACCES;
>
> --
> 2.15.0
>
This patch breaks the kernel build on RHEL b/c it generates
a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
That's b/c the patch adds real_qp->qp_sec to if's conditions,
and the compiler cannot determine if real_qp->qp_sec cannot be modified
between the first check like it, above, which sets the value of new_pps,
and the second check that uses it, because real_qp is passed into the device->modify()
function call btwn those two if() check's.
The code needs to do something like this in the first if-check:
.....
bool new_pps_gotten = false;
....
if (pps_change && !special_qp && real_qp->qp_sec) {
mutex_lock(&real_qp->qp_sec->mutex);
new_pps = get_new_pps(real_qp,
qp_attr,
qp_attr_mask);
new_pps_gotten = true;
....
}
....
and change the second if check to be:
if (new_pps_gotten) {
* Clean up the lists and free the appropriate
.....
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-27 22:03 ` Don Dutile
@ 2017-11-27 22:58 ` Daniel Jurgens
2017-11-27 23:28 ` Don Dutile
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Jurgens @ 2017-11-27 22:58 UTC (permalink / raw)
To: Don Dutile, Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Paul Moore, stable
On 11/27/2017 4:03 PM, Don Dutile wrote:
> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
>> From: Daniel Jurgens <danielj@mellanox.com>
>>
>>
>> -��� if (pps_change && !special_qp) {
>> +��� if (pps_change && !special_qp && real_qp->qp_sec) {
>> �������� mutex_lock(&real_qp->qp_sec->mutex);
>> �������� new_pps = get_new_pps(real_qp,
>> ���������������������� qp_attr,
>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
>> ������������������������� qp_attr_mask,
>> ������������������������� udata);
>>
>> -��� if (pps_change && !special_qp) {
>> +��� if (pps_change && !special_qp && real_qp->qp_sec) {
>> �������� /* Clean up the lists and free the appropriate
>> ��������� * ports_pkeys structure.
>> ��������� */
>>
> This patch breaks the kernel build on RHEL b/c it generates
> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
>
> That's b/c the patch adds real_qp->qp_sec to if's conditions,
> and� the compiler cannot determine if real_qp->qp_sec cannot be modified
> between the first check like it, above, which sets the value of new_pps,
> and the second check that uses it, because real_qp is passed into the device->modify()
> function call btwn those two if() check's.
>
> The code needs to do something like this in the first if-check:
> �� .....
> bool new_pps_gotten = false;
> �� ....
>
> if (pps_change && !special_qp && real_qp->qp_sec) {
> ����mutex_lock(&real_qp->qp_sec->mutex);
> ����new_pps = get_new_pps(real_qp,
> ����������������� qp_attr,
> ����������������� qp_attr_mask);
> ����new_pps_gotten = true;
> ������� ....
> }
> ���� ....
>
> and change the second if check to be:
>
> if (new_pps_gotten) {
> ����* Clean up the lists and free the appropriate
> ���� .....
>
Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-27 22:58 ` Daniel Jurgens
@ 2017-11-27 23:28 ` Don Dutile
2017-11-28 20:38 ` Don Dutile
0 siblings, 1 reply; 7+ messages in thread
From: Don Dutile @ 2017-11-27 23:28 UTC (permalink / raw)
To: Daniel Jurgens, Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Paul Moore, stable
On 11/27/2017 05:58 PM, Daniel Jurgens wrote:
> On 11/27/2017 4:03 PM, Don Dutile wrote:
>> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
>>> From: Daniel Jurgens <danielj@mellanox.com>
>>>
>>>
>>> - if (pps_change && !special_qp) {
>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>>> mutex_lock(&real_qp->qp_sec->mutex);
>>> new_pps = get_new_pps(real_qp,
>>> qp_attr,
>>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
>>> qp_attr_mask,
>>> udata);
>>>
>>> - if (pps_change && !special_qp) {
>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>>> /* Clean up the lists and free the appropriate
>>> * ports_pkeys structure.
>>> */
>>>
>> This patch breaks the kernel build on RHEL b/c it generates
>> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
>> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
>>
>> That's b/c the patch adds real_qp->qp_sec to if's conditions,
>> and the compiler cannot determine if real_qp->qp_sec cannot be modified
>> between the first check like it, above, which sets the value of new_pps,
>> and the second check that uses it, because real_qp is passed into the device->modify()
>> function call btwn those two if() check's.
>>
>> The code needs to do something like this in the first if-check:
>> .....
>> bool new_pps_gotten = false;
>> ....
>>
>> if (pps_change && !special_qp && real_qp->qp_sec) {
>> mutex_lock(&real_qp->qp_sec->mutex);
>> new_pps = get_new_pps(real_qp,
>> qp_attr,
>> qp_attr_mask);
>> new_pps_gotten = true;
>> ....
>> }
>> ....
>>
>> and change the second if check to be:
>>
>> if (new_pps_gotten) {
>> * Clean up the lists and free the appropriate
>> .....
>>
>
> Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.
>
yup, I considered that as well.
wasn't sure if lockdep checking code would not like the fact that a mutex_lock() could be taken,
but if new_pps == NULL after the get call(it may always succeed, but an analyzer may not conclude the same),
that the mutex_unlock() wouldn't be called.
the double, same-condition if-check with the fcn call in btwn seems like it ought to be
restructured differently so the mutex lock/unlock pairs are contained neatly in a single if-clause,
and the new_pps alloc & use would be similarly containted.
-dd
-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-27 23:28 ` Don Dutile
@ 2017-11-28 20:38 ` Don Dutile
2017-11-28 21:03 ` Daniel Jurgens
0 siblings, 1 reply; 7+ messages in thread
From: Don Dutile @ 2017-11-28 20:38 UTC (permalink / raw)
To: Daniel Jurgens, Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Paul Moore, stable
On 11/27/2017 06:28 PM, Don Dutile wrote:
> On 11/27/2017 05:58 PM, Daniel Jurgens wrote:
>> On 11/27/2017 4:03 PM, Don Dutile wrote:
>>> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
>>>> From: Daniel Jurgens <danielj@mellanox.com>
>>>>
>>>>
>>>> - if (pps_change && !special_qp) {
>>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>>>> mutex_lock(&real_qp->qp_sec->mutex);
>>>> new_pps = get_new_pps(real_qp,
>>>> qp_attr,
>>>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
>>>> qp_attr_mask,
>>>> udata);
>>>>
>>>> - if (pps_change && !special_qp) {
>>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
>>>> /* Clean up the lists and free the appropriate
>>>> * ports_pkeys structure.
>>>> */
>>>>
>>> This patch breaks the kernel build on RHEL b/c it generates
>>> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
>>> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
>>>
>>> That's b/c the patch adds real_qp->qp_sec to if's conditions,
>>> and the compiler cannot determine if real_qp->qp_sec cannot be modified
>>> between the first check like it, above, which sets the value of new_pps,
>>> and the second check that uses it, because real_qp is passed into the device->modify()
>>> function call btwn those two if() check's.
>>>
>>> The code needs to do something like this in the first if-check:
>>> .....
>>> bool new_pps_gotten = false;
>>> ....
>>>
>>> if (pps_change && !special_qp && real_qp->qp_sec) {
>>> mutex_lock(&real_qp->qp_sec->mutex);
>>> new_pps = get_new_pps(real_qp,
>>> qp_attr,
>>> qp_attr_mask);
>>> new_pps_gotten = true;
>>> ....
>>> }
>>> ....
>>>
>>> and change the second if check to be:
>>>
>>> if (new_pps_gotten) {
>>> * Clean up the lists and free the appropriate
>>> .....
>>>
>>
>> Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.
>>
> yup, I considered that as well.
> wasn't sure if lockdep checking code would not like the fact that a mutex_lock() could be taken,
> but if new_pps == NULL after the get call(it may always succeed, but an analyzer may not conclude the same),
> that the mutex_unlock() wouldn't be called.
>
> the double, same-condition if-check with the fcn call in btwn seems like it ought to be
> restructured differently so the mutex lock/unlock pairs are contained neatly in a single if-clause,
> and the new_pps alloc & use would be similarly containted.
>
> -dd
>
> -
Is someone doing a v3? I didn't see an email today w/another patch version.
... at least I wasn't directly cc'd on one anyhow... off to check linux-rdma folder...
nothing there... and I don't find a v3 in Leon's tree either.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-28 20:38 ` Don Dutile
@ 2017-11-28 21:03 ` Daniel Jurgens
2017-11-29 5:11 ` Leon Romanovsky
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Jurgens @ 2017-11-28 21:03 UTC (permalink / raw)
To: Don Dutile, Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: linux-rdma, Paul Moore, stable
On 11/28/2017 2:38 PM, Don Dutile wrote:
> On 11/27/2017 06:28 PM, Don Dutile wrote:
>> On 11/27/2017 05:58 PM, Daniel Jurgens wrote:
>>> On 11/27/2017 4:03 PM, Don Dutile wrote:
>>>> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
>>>>> From: Daniel Jurgens <danielj@mellanox.com>
>>>>>
>>>>>
>>>>> -��� if (pps_change && !special_qp) {
>>>>> +��� if (pps_change && !special_qp && real_qp->qp_sec) {
>>>>> �������� mutex_lock(&real_qp->qp_sec->mutex);
>>>>> �������� new_pps = get_new_pps(real_qp,
>>>>> ���������������������� qp_attr,
>>>>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
>>>>> ������������������������� qp_attr_mask,
>>>>> ������������������������� udata);
>>>>>
>>>>> -��� if (pps_change && !special_qp) {
>>>>> +��� if (pps_change && !special_qp && real_qp->qp_sec) {
>>>>> �������� /* Clean up the lists and free the appropriate
>>>>> ��������� * ports_pkeys structure.
>>>>> ��������� */
>>>>>
>>>> This patch breaks the kernel build on RHEL b/c it generates
>>>> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
>>>> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
>>>>
>>>> That's b/c the patch adds real_qp->qp_sec to if's conditions,
>>>> and� the compiler cannot determine if real_qp->qp_sec cannot be modified
>>>> between the first check like it, above, which sets the value of new_pps,
>>>> and the second check that uses it, because real_qp is passed into the device->modify()
>>>> function call btwn those two if() check's.
>>>>
>>>> The code needs to do something like this in the first if-check:
>>>> �� .....
>>>> bool new_pps_gotten = false;
>>>> �� ....
>>>>
>>>> if (pps_change && !special_qp && real_qp->qp_sec) {
>>>> ��� mutex_lock(&real_qp->qp_sec->mutex);
>>>> ��� new_pps = get_new_pps(real_qp,
>>>> ����������������� qp_attr,
>>>> ����������������� qp_attr_mask);
>>>> ��� new_pps_gotten = true;
>>>> ������� ....
>>>> }
>>>> ���� ....
>>>>
>>>> and change the second if check to be:
>>>>
>>>> if (new_pps_gotten) {
>>>> ��� * Clean up the lists and free the appropriate
>>>> ���� .....
>>>>
>>>
>>> Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.
>>>
>> yup, I considered that as well.
>> wasn't sure if lockdep checking code would not like the fact that a mutex_lock() could be taken,
>> but if new_pps == NULL after the get call(it may always succeed, but an analyzer may not conclude the same),
>> that the mutex_unlock() wouldn't be called.
>>
>> the double, same-condition if-check with the fcn call in btwn seems like it ought to be
>> restructured differently so the mutex lock/unlock pairs are contained neatly in a single if-clause,
>> and the new_pps alloc & use would be similarly containted.
>>
>> -dd
>>
>> -
> Is someone doing a v3? I didn't see an email today w/another patch version.
> ... at least I wasn't directly cc'd on one anyhow... off to check linux-rdma folder...
> nothing there... and I don't find a v3 in Leon's tree either.
>
>
I sent a fix-up patch to Leon, but it seems he didn't get to it today. I'll resend this afternoon.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand
2017-11-28 21:03 ` Daniel Jurgens
@ 2017-11-29 5:11 ` Leon Romanovsky
0 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2017-11-29 5:11 UTC (permalink / raw)
To: Daniel Jurgens
Cc: Don Dutile, Doug Ledford, Jason Gunthorpe, linux-rdma, Paul Moore,
stable
[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]
On Tue, Nov 28, 2017 at 03:03:39PM -0600, Daniel Jurgens wrote:
> On 11/28/2017 2:38 PM, Don Dutile wrote:
> > On 11/27/2017 06:28 PM, Don Dutile wrote:
> >> On 11/27/2017 05:58 PM, Daniel Jurgens wrote:
> >>> On 11/27/2017 4:03 PM, Don Dutile wrote:
> >>>> On 11/27/2017 06:25 AM, Leon Romanovsky wrote:
> >>>>> From: Daniel Jurgens <danielj@mellanox.com>
> >>>>>
> >>>>>
> >>>>> - if (pps_change && !special_qp) {
> >>>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
> >>>>> mutex_lock(&real_qp->qp_sec->mutex);
> >>>>> new_pps = get_new_pps(real_qp,
> >>>>> qp_attr,
> >>>>> @@ -600,7 +627,7 @@ int ib_security_modify_qp(struct ib_qp *qp,
> >>>>> qp_attr_mask,
> >>>>> udata);
> >>>>>
> >>>>> - if (pps_change && !special_qp) {
> >>>>> + if (pps_change && !special_qp && real_qp->qp_sec) {
> >>>>> /* Clean up the lists and free the appropriate
> >>>>> * ports_pkeys structure.
> >>>>> */
> >>>>>
> >>>> This patch breaks the kernel build on RHEL b/c it generates
> >>>> a warning in the second if (pps_change && !special_qp && real_qp->qp_sec) {}
> >>>> that new_pps may not be assigned. ... build warnings in RHEL kernel == build failure (on x86).
> >>>>
> >>>> That's b/c the patch adds real_qp->qp_sec to if's conditions,
> >>>> and the compiler cannot determine if real_qp->qp_sec cannot be modified
> >>>> between the first check like it, above, which sets the value of new_pps,
> >>>> and the second check that uses it, because real_qp is passed into the device->modify()
> >>>> function call btwn those two if() check's.
> >>>>
> >>>> The code needs to do something like this in the first if-check:
> >>>> .....
> >>>> bool new_pps_gotten = false;
> >>>> ....
> >>>>
> >>>> if (pps_change && !special_qp && real_qp->qp_sec) {
> >>>> mutex_lock(&real_qp->qp_sec->mutex);
> >>>> new_pps = get_new_pps(real_qp,
> >>>> qp_attr,
> >>>> qp_attr_mask);
> >>>> new_pps_gotten = true;
> >>>> ....
> >>>> }
> >>>> ....
> >>>>
> >>>> and change the second if check to be:
> >>>>
> >>>> if (new_pps_gotten) {
> >>>> * Clean up the lists and free the appropriate
> >>>> .....
> >>>>
> >>>
> >>> Thanks Don, I think it's better to initialize new_pps to NULL, vs introducing a new variable. Also, there needs to be a check of new_pps after getting it.
> >>>
> >> yup, I considered that as well.
> >> wasn't sure if lockdep checking code would not like the fact that a mutex_lock() could be taken,
> >> but if new_pps == NULL after the get call(it may always succeed, but an analyzer may not conclude the same),
> >> that the mutex_unlock() wouldn't be called.
> >>
> >> the double, same-condition if-check with the fcn call in btwn seems like it ought to be
> >> restructured differently so the mutex lock/unlock pairs are contained neatly in a single if-clause,
> >> and the new_pps alloc & use would be similarly containted.
> >>
> >> -dd
> >>
> >> -
> > Is someone doing a v3? I didn't see an email today w/another patch version.
> > ... at least I wasn't directly cc'd on one anyhow... off to check linux-rdma folder...
> > nothing there... and I don't find a v3 in Leon's tree either.
> >
> >
> I sent a fix-up patch to Leon, but it seems he didn't get to it today. I'll resend this afternoon.
Thanks Daniel for handling this.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-29 5:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-27 11:25 [PATCH rdma-rc v2] IB/core: Only enforce security for InfiniBand Leon Romanovsky
2017-11-27 22:03 ` Don Dutile
2017-11-27 22:58 ` Daniel Jurgens
2017-11-27 23:28 ` Don Dutile
2017-11-28 20:38 ` Don Dutile
2017-11-28 21:03 ` Daniel Jurgens
2017-11-29 5:11 ` Leon Romanovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).