[RFC] [PATCH] can: fix handling of unmodifiable configuration options

[RFC] [PATCH] can: fix handling of unmodifiable configuration options

[Oliver Hartkopp]

As described in 'can: m_can: tag current CAN FD controllers as non-ISO'
(6cfda7fbebe) it is possible to define fixed configuration options by
setting the according bit in 'ctrlmode' and clear it in 'ctrlmode_supported'.
This leads to the incovenience that the fixed configuration can not be passed
by netlink (ip tool) even when it has the correct value (e.g. non-ISO, FD).

This patch fixes that issue and allows fixed set bit values to be set again
which does not change the unmodifiable content.

Additionally it fixes the inconsitency that was prohibiting the support of
'CANFD-only' controller drivers.

Reported-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-stable <stable@vger.kernel.org> 3.16+
---
 drivers/net/can/dev.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 141c2a4..b084cfd 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -696,11 +696,18 @@ int can_change_mtu(struct net_device *dev, int new_mtu)
 	/* allow change of MTU according to the CANFD ability of the device */
 	switch (new_mtu) {
 	case CAN_MTU:
+		/* take care of 'CANFD-only' controllers */
+		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
+		    (priv->ctrlmode & CAN_CTRLMODE_FD))
+			return -EINVAL;
+
 		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
 		break;
 
 	case CANFD_MTU:
-		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
+		/* check for CANFD capability */
+		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
+		    !(priv->ctrlmode & CAN_CTRLMODE_FD))
 			return -EINVAL;
 
 		priv->ctrlmode |= CAN_CTRLMODE_FD;
@@ -819,10 +826,13 @@ static int can_changelink(struct net_device *dev,
 			return -EBUSY;
 		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
 
-		/* check whether changed bits are allowed to be modified */
-		if (cm->mask & ~priv->ctrlmode_supported)
+		/* check whether provided bits are allowed to be passed */
+		if (cm->mask & ~(priv->ctrlmode_supported | priv->ctrlmode))
 			return -EOPNOTSUPP;
 
+		/* reduce to bits that are allowed to be modified */
+		cm->mask &= priv->ctrlmode_supported;
+
 		/* clear bits to be modified and copy the flag values */
 		priv->ctrlmode &= ~cm->mask;
 		priv->ctrlmode |= (cm->flags & cm->mask);
-- 
2.7.0

Re: [RFC] [PATCH] can: fix handling of unmodifiable configuration options

[Oliver Hartkopp]

On 03/08/2016 06:13 PM, Oliver Hartkopp wrote:
> As described in 'can: m_can: tag current CAN FD controllers as non-ISO'
> (6cfda7fbebe) it is possible to define fixed configuration options by
> setting the according bit in 'ctrlmode' and clear it in 'ctrlmode_supported'.
> This leads to the incovenience that the fixed configuration can not be passed
> by netlink (ip tool) even when it has the correct value (e.g. non-ISO, FD).
> 
> This patch fixes that issue and allows fixed set bit values to be set again
> which does not change the unmodifiable content.
> 
> Additionally it fixes the inconsitency that was prohibiting the support of
> 'CANFD-only' controller drivers.
> 
> Reported-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: linux-stable <stable@vger.kernel.org> 3.16+
> ---
>  drivers/net/can/dev.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 141c2a4..b084cfd 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -696,11 +696,18 @@ int can_change_mtu(struct net_device *dev, int new_mtu)
>  	/* allow change of MTU according to the CANFD ability of the device */
>  	switch (new_mtu) {
>  	case CAN_MTU:
> +		/* take care of 'CANFD-only' controllers */
> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
> +		    (priv->ctrlmode & CAN_CTRLMODE_FD))
> +			return -EINVAL;
> +
>  		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
>  		break;
>  
>  	case CANFD_MTU:
> -		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
> +		/* check for CANFD capability */
> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
> +		    !(priv->ctrlmode & CAN_CTRLMODE_FD))
>  			return -EINVAL;
>  
>  		priv->ctrlmode |= CAN_CTRLMODE_FD;
> @@ -819,10 +826,13 @@ static int can_changelink(struct net_device *dev,
>  			return -EBUSY;
>  		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
>  
> -		/* check whether changed bits are allowed to be modified */
> -		if (cm->mask & ~priv->ctrlmode_supported)
> +		/* check whether provided bits are allowed to be passed */
> +		if (cm->mask & ~(priv->ctrlmode_supported | priv->ctrlmode))
>  			return -EOPNOTSUPP;
>  
> +		/* reduce to bits that are allowed to be modified */
> +		cm->mask &= priv->ctrlmode_supported;
> +

I just realized that I'm updating a provided netlink attribute here.
Does anyone know, whether this is feasible OR if I need to spend an extra
variable to perform the '&=' operation?

Are netlink attributes to be treated as constants?

Thanks,
Oliver

>  		/* clear bits to be modified and copy the flag values */
>  		priv->ctrlmode &= ~cm->mask;
>  		priv->ctrlmode |= (cm->flags & cm->mask);
> 

RE: [RFC] [PATCH] can: fix handling of unmodifiable configuration options

[Ramesh Shanmugasundaram]

Hi Oliver,

Thanks for the patch.

> -----Original Message-----
> From: Oliver Hartkopp [mailto:socketcan@hartkopp.net]
> Sent: 08 March 2016 17:14
> To: linux-can@vger.kernel.org
> Cc: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>;
> Oliver Hartkopp <socketcan@hartkopp.net>; linux-stable 3 . 16+
> <stable@vger.kernel.org>
> Subject: [RFC] [PATCH] can: fix handling of unmodifiable configuration
> options
> 
> As described in 'can: m_can: tag current CAN FD controllers as non-ISO'
> (6cfda7fbebe) it is possible to define fixed configuration options by
> setting the according bit in 'ctrlmode' and clear it in
> 'ctrlmode_supported'.

Shouldn't we document this? A comment to these variable definitions perhaps?

> This leads to the incovenience that the fixed configuration can not be
> passed by netlink (ip tool) even when it has the correct value (e.g. non-
> ISO, FD).
> 
> This patch fixes that issue and allows fixed set bit values to be set
> again which does not change the unmodifiable content.
> 
> Additionally it fixes the inconsitency that was prohibiting the support of
> 'CANFD-only' controller drivers.
> 
> Reported-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: linux-stable <stable@vger.kernel.org> 3.16+
> ---
>  drivers/net/can/dev.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index
> 141c2a4..b084cfd 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -696,11 +696,18 @@ int can_change_mtu(struct net_device *dev, int
> new_mtu)
>  	/* allow change of MTU according to the CANFD ability of the device
> */
>  	switch (new_mtu) {
>  	case CAN_MTU:
> +		/* take care of 'CANFD-only' controllers */
> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
> +		    (priv->ctrlmode & CAN_CTRLMODE_FD))
> +			return -EINVAL;
> +
>  		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
>  		break;
> 
>  	case CANFD_MTU:
> -		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
> +		/* check for CANFD capability */
> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
> +		    !(priv->ctrlmode & CAN_CTRLMODE_FD))
>  			return -EINVAL;
> 
>  		priv->ctrlmode |= CAN_CTRLMODE_FD;
> @@ -819,10 +826,13 @@ static int can_changelink(struct net_device *dev,
>  			return -EBUSY;
>  		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
> 
> -		/* check whether changed bits are allowed to be modified */
> -		if (cm->mask & ~priv->ctrlmode_supported)
> +		/* check whether provided bits are allowed to be passed */
> +		if (cm->mask & ~(priv->ctrlmode_supported | priv->ctrlmode))
>  			return -EOPNOTSUPP;

I think we need to separate this check into two. Otherwise, it will still return success for "fd off" config even though the configuration is not really applied.

> 
> +		/* reduce to bits that are allowed to be modified */
> +		cm->mask &= priv->ctrlmode_supported;

If we separate the above check, I think we don't have to do this.

Thanks,
Ramesh

Re: [RFC] [PATCH] can: fix handling of unmodifiable configuration options

[Oliver Hartkopp]

On 03/09/2016 10:27 AM, Ramesh Shanmugasundaram wrote:
> Hi Oliver,
> 
> Thanks for the patch.
> 
>> -----Original Message-----
>> From: Oliver Hartkopp [mailto:socketcan@hartkopp.net]
>> Sent: 08 March 2016 17:14
>> To: linux-can@vger.kernel.org
>> Cc: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>;
>> Oliver Hartkopp <socketcan@hartkopp.net>; linux-stable 3 . 16+
>> <stable@vger.kernel.org>
>> Subject: [RFC] [PATCH] can: fix handling of unmodifiable configuration
>> options
>>
>> As described in 'can: m_can: tag current CAN FD controllers as non-ISO'
>> (6cfda7fbebe) it is possible to define fixed configuration options by
>> setting the according bit in 'ctrlmode' and clear it in
>> 'ctrlmode_supported'.
> 
> Shouldn't we document this? A comment to these variable definitions perhaps?

Yes. Good idea. Will do this in the updated patch.

> 
>> This leads to the incovenience that the fixed configuration can not be
>> passed by netlink (ip tool) even when it has the correct value (e.g. non-
>> ISO, FD).
>>
>> This patch fixes that issue and allows fixed set bit values to be set
>> again which does not change the unmodifiable content.
>>
>> Additionally it fixes the inconsitency that was prohibiting the support of
>> 'CANFD-only' controller drivers.
>>
>> Reported-by: Ramesh Shanmugasundaram
>> <ramesh.shanmugasundaram@bp.renesas.com>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Cc: linux-stable <stable@vger.kernel.org> 3.16+
>> ---
>>  drivers/net/can/dev.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index
>> 141c2a4..b084cfd 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -696,11 +696,18 @@ int can_change_mtu(struct net_device *dev, int
>> new_mtu)
>>  	/* allow change of MTU according to the CANFD ability of the device
>> */
>>  	switch (new_mtu) {
>>  	case CAN_MTU:
>> +		/* take care of 'CANFD-only' controllers */
>> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
>> +		    (priv->ctrlmode & CAN_CTRLMODE_FD))
>> +			return -EINVAL;
>> +
>>  		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
>>  		break;
>>
>>  	case CANFD_MTU:
>> -		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
>> +		/* check for CANFD capability */
>> +		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
>> +		    !(priv->ctrlmode & CAN_CTRLMODE_FD))
>>  			return -EINVAL;
>>
>>  		priv->ctrlmode |= CAN_CTRLMODE_FD;
>> @@ -819,10 +826,13 @@ static int can_changelink(struct net_device *dev,
>>  			return -EBUSY;
>>  		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
>>
>> -		/* check whether changed bits are allowed to be modified */
>> -		if (cm->mask & ~priv->ctrlmode_supported)
>> +		/* check whether provided bits are allowed to be passed */
>> +		if (cm->mask & ~(priv->ctrlmode_supported | priv->ctrlmode))
>>  			return -EOPNOTSUPP;
> 
> I think we need to separate this check into two. Otherwise, it will still return success for "fd off" config even though the configuration is not really applied.

Yes. I'll re-think this check. Your "fd off" use-case is a good case to take
care about.

> 
>>
>> +		/* reduce to bits that are allowed to be modified */
>> +		cm->mask &= priv->ctrlmode_supported;
> 
> If we separate the above check, I think we don't have to do this.

At least I'll make it a separate variable, so that the netlink data remains
read-only.

Thanks,
Oliver