Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable

Martin Schiller
2020-01-14T05:37:09Z

On 2020-01-13 14:53, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 13:45:50 +0100, Martin Schiller wrote:
>> This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2,
>> N2 via
>> sethdlc (which needs to be patched as well).
>>
>> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
>
>> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
>> index 5643675ff724..b28051eba736 100644
>> --- a/drivers/net/wan/hdlc_x25.c
>> +++ b/drivers/net/wan/hdlc_x25.c
>> @@ -21,8 +21,17 @@
>> #include <linux/skbuff.h>
>> #include <net/x25device.h>
>>
>> +struct x25_state {
>> + x25_hdlc_proto settings;
>> +};
>> +
>> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
>>
>> +static inline struct x25_state* state(hdlc_device *hdlc)
>
> Please no more static inlines in source files. Compiler will know what
> to do.

OK, i will remove the "inline". I've just used it, because it's also
in the hdlc_fr.c and hdlc_cisco.c code.

>
>> +{
>> + return (struct x25_state *)hdlc->state;
>> +}
>> +
>> /* These functions are callbacks called by LAPB layer */
>>
>> static void x25_connect_disconnect(struct net_device *dev, int
>> reason, int code)
>
>> @@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
>>
>> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>> {
>> + x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
>> + const size_t size = sizeof(x25_hdlc_proto);
>> + x25_hdlc_proto new_settings;
>> hdlc_device *hdlc = dev_to_hdlc(dev);
>> int result;
>>
>> @@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev,
>> struct ifreq *ifr)
>> if (dev_to_hdlc(dev)->proto != &proto)
>> return -EINVAL;
>> ifr->ifr_settings.type = IF_PROTO_X25;
>> - return 0; /* return protocol only, no settable parameters */
>> + if (ifr->ifr_settings.size < size) {
>> + ifr->ifr_settings.size = size; /* data size wanted */
>> + return -ENOBUFS;
>> + }
>> + if (copy_to_user(x25_s, &state(hdlc)->settings, size))
>> + return -EFAULT;
>> + return 0;
>>
>> case IF_PROTO_X25:
>> if (!capable(CAP_NET_ADMIN))
>> @@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev,
>> struct ifreq *ifr)
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>>
>> + if (copy_from_user(&new_settings, x25_s, size))
>> + return -EFAULT;
>> +
>> + if ((new_settings.dce != 0 &&
>> + new_settings.dce != 1) ||
>> + (new_settings.modulo != 8 &&
>> + new_settings.modulo != 128) ||
>> + new_settings.window < 1 ||
>> + (new_settings.modulo == 8 &&
>> + new_settings.window > 7) ||
>> + (new_settings.modulo == 128 &&
>> + new_settings.window > 127) ||
>> + new_settings.t1 < 1 ||
>> + new_settings.t1 > 255 ||
>> + new_settings.t2 < 1 ||
>> + new_settings.t2 > 255 ||
>> + new_settings.n2 < 1 ||
>> + new_settings.n2 > 255)
>> + return -EINVAL;
>> +
>> result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
>> if (result)
>> return result;
>>
>> - if ((result = attach_hdlc_protocol(dev, &proto, 0)))
>> + if ((result = attach_hdlc_protocol(dev, &proto,
>> + sizeof(struct x25_state))))
>> return result;
>> +
>> + memcpy(&state(hdlc)->settings, &new_settings, size);
>> dev->type = ARPHRD_X25;
>> call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
>> netif_dormant_off(dev);
>> diff --git a/include/uapi/linux/hdlc/ioctl.h
>> b/include/uapi/linux/hdlc/ioctl.h
>> index 0fe4238e8246..3656ce8b8af0 100644
>> --- a/include/uapi/linux/hdlc/ioctl.h
>> +++ b/include/uapi/linux/hdlc/ioctl.h
>> @@ -3,7 +3,7 @@
>> #define __HDLC_IOCTL_H__
>>
>>
>> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc
>> utility */
>> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc
>> utility */
>
> What's the backward compatibility story in this code?

Well, I thought I have to increment the version to keep the kernel code
and the sethdlc utility in sync (like the comment says).

>
> The IOCTL handling at least looks like it may start returning errors
> to existing user space which could have expected the parameters to
> IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.

I could also try to implement it without incrementing the version by
looking at ifr_settings.size and using the former defaults if the size
doesn't match.

>
>> #define CLOCK_DEFAULT 0 /* Default setting */
>> #define CLOCK_EXT 1 /* External TX and RX clock - DTE */