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

[Martin Schiller]

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@xxxxxxxxxx>
>
> > 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 */