| Message ID | 20200113124551.2570-1-ms@dev.tdt.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] wan/hdlc_x25: make lapb params configurable | expand |
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. > +{ > + 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? 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. > #define CLOCK_DEFAULT 0 /* Default setting */ > #define CLOCK_EXT 1 /* External TX and RX clock - DTE */
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 */
On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote: > >> 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). Perhaps I chose the wrong place for asking this question, IOCTL code was my real worry. I don't think this version number is validated so I think bumping it shouldn't break anything? > > 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. Sounds good, thank you!
On 2020-01-14 13:51, Jakub Kicinski wrote: > On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote: >> >> 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). > > Perhaps I chose the wrong place for asking this question, IOCTL code > was my real worry. I don't think this version number is validated so > I think bumping it shouldn't break anything? sethdlc validates the GENERIC_HDLC_VERSION at compile time. https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/ Another question: Where do I have to send my patch for sethdlc to? > >> > 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. > > Sounds good, thank you! OK, I will send a v2 of the patch.
On Tue, 14 Jan 2020 14:33:51 +0100, Martin Schiller wrote: > On 2020-01-14 13:51, Jakub Kicinski wrote: > > On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote: > >> >> 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). > > > > Perhaps I chose the wrong place for asking this question, IOCTL code > > was my real worry. I don't think this version number is validated so > > I think bumping it shouldn't break anything? > > sethdlc validates the GENERIC_HDLC_VERSION at compile time. > > https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/ Aw, okay, best not to bump it then. > Another question: > Where do I have to send my patch for sethdlc to? No idea :)
Hi, I overlooked this mail, sorry. Martin Schiller <ms@dev.tdt.de> writes: > sethdlc validates the GENERIC_HDLC_VERSION at compile time. > > https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/ > > Another question: > Where do I have to send my patch for sethdlc to? I guess you have to put your own version somewhere. I don't have access to hdlc directory at kernel.org anymore, this stuff (sync serial with HDLC) is no longer in use around here.
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) +{ + 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) @@ -132,6 +141,8 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev) static int x25_open(struct net_device *dev) { int result; + hdlc_device *hdlc = dev_to_hdlc(dev); + struct lapb_parms_struct params; static const struct lapb_register_struct cb = { .connect_confirmation = x25_connected, .connect_indication = x25_connected, @@ -144,6 +155,26 @@ static int x25_open(struct net_device *dev) result = lapb_register(dev, &cb); if (result != LAPB_OK) return result; + + result = lapb_getparms(dev, ¶ms); + if (result != LAPB_OK) + return result; + + if (state(hdlc)->settings.dce) + params.mode = params.mode | LAPB_DCE; + + if (state(hdlc)->settings.modulo == 128) + params.mode = params.mode | LAPB_EXTENDED; + + params.window = state(hdlc)->settings.window; + params.t1 = state(hdlc)->settings.t1; + params.t2 = state(hdlc)->settings.t2; + params.n2 = state(hdlc)->settings.n2; + + result = lapb_setparms(dev, ¶ms); + if (result != LAPB_OK) + return result; + return 0; } @@ -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 */ #define CLOCK_DEFAULT 0 /* Default setting */ #define CLOCK_EXT 1 /* External TX and RX clock - DTE */ @@ -79,6 +79,15 @@ typedef struct { unsigned int timeout; } cisco_proto; +typedef struct { + unsigned short dce; /* 1 for DCE (network side) operation */ + unsigned int modulo; /* modulo (8 = basic / 128 = extended) */ + unsigned int window; /* frame window size */ + unsigned int t1; /* timeout t1 */ + unsigned int t2; /* timeout t2 */ + unsigned int n2; /* frame retry counter */ +} x25_hdlc_proto; + /* PPP doesn't need any info now - supply length = 0 to ioctl */ #endif /* __ASSEMBLY__ */ diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h index 4bf33344aab1..be714cd8c826 100644 --- a/include/uapi/linux/if.h +++ b/include/uapi/linux/if.h @@ -213,6 +213,7 @@ struct if_settings { fr_proto __user *fr; fr_proto_pvc __user *fr_pvc; fr_proto_pvc_info __user *fr_pvc_info; + x25_hdlc_proto __user *x25; /* interface settings */ sync_serial_settings __user *sync;
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> --- drivers/net/wan/hdlc_x25.c | 67 ++++++++++++++++++++++++++++++++- include/uapi/linux/hdlc/ioctl.h | 11 +++++- include/uapi/linux/if.h | 1 + 3 files changed, 76 insertions(+), 3 deletions(-)