| Message ID | 20201204133508.742120-4-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/3] can: softing: softing_netdev_open(): fix error handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Pull request |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 82 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Fri, 4 Dec 2020 14:35:08 +0100 Marc Kleine-Budde wrote: > From: Oliver Hartkopp <socketcan@hartkopp.net> > > When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP > socket is switched into functional addressing mode, where only single frame > (SF) protocol data units can be send on the specified CAN interface and the > given tp.tx_id after bind(). > > In opposite to normal and extended addressing this socket does not register a > CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a > segmented bi-directional data transfer. > > Sending SFs on this socket is therefore a TX-only 'broadcast' operation. Unclear from this patch what is getting fixed. Looks a little bit like a feature which could be added in a backward compatible way, no? Is it only added for completeness of the ISOTP implementation?
On 05.12.20 04:44, Jakub Kicinski wrote: > On Fri, 4 Dec 2020 14:35:08 +0100 Marc Kleine-Budde wrote: >> From: Oliver Hartkopp <socketcan@hartkopp.net> >> >> When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP >> socket is switched into functional addressing mode, where only single frame >> (SF) protocol data units can be send on the specified CAN interface and the >> given tp.tx_id after bind(). >> >> In opposite to normal and extended addressing this socket does not register a >> CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a >> segmented bi-directional data transfer. >> >> Sending SFs on this socket is therefore a TX-only 'broadcast' operation. > > Unclear from this patch what is getting fixed. Looks a little bit like > a feature which could be added in a backward compatible way, no? > Is it only added for completeness of the ISOTP implementation? > Yes, the latter. It's a very small and simple tested addition and I hope it can still go into the initial upstream process. Many thanks, Oliver
On 12/5/20 12:26 PM, Oliver Hartkopp wrote: > > > On 05.12.20 04:44, Jakub Kicinski wrote: >> On Fri, 4 Dec 2020 14:35:08 +0100 Marc Kleine-Budde wrote: >>> From: Oliver Hartkopp <socketcan@hartkopp.net> >>> >>> When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP >>> socket is switched into functional addressing mode, where only single frame >>> (SF) protocol data units can be send on the specified CAN interface and the >>> given tp.tx_id after bind(). >>> >>> In opposite to normal and extended addressing this socket does not register a >>> CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a >>> segmented bi-directional data transfer. >>> >>> Sending SFs on this socket is therefore a TX-only 'broadcast' operation. >> >> Unclear from this patch what is getting fixed. Looks a little bit like >> a feature which could be added in a backward compatible way, no? >> Is it only added for completeness of the ISOTP implementation? >> > > Yes, the latter. > > It's a very small and simple tested addition and I hope it can still go > into the initial upstream process. What about the (incremental?) change that Thomas Wagner posted? https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de Marc
On Sat, 5 Dec 2020 21:24:42 +0100 Marc Kleine-Budde wrote: > On 12/5/20 12:26 PM, Oliver Hartkopp wrote: > > On 05.12.20 04:44, Jakub Kicinski wrote: > >> On Fri, 4 Dec 2020 14:35:08 +0100 Marc Kleine-Budde wrote: > >>> From: Oliver Hartkopp <socketcan@hartkopp.net> > >>> > >>> When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP > >>> socket is switched into functional addressing mode, where only single frame > >>> (SF) protocol data units can be send on the specified CAN interface and the > >>> given tp.tx_id after bind(). > >>> > >>> In opposite to normal and extended addressing this socket does not register a > >>> CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a > >>> segmented bi-directional data transfer. > >>> > >>> Sending SFs on this socket is therefore a TX-only 'broadcast' operation. > >> > >> Unclear from this patch what is getting fixed. Looks a little bit like > >> a feature which could be added in a backward compatible way, no? > >> Is it only added for completeness of the ISOTP implementation? > >> > > > > Yes, the latter. > > > > It's a very small and simple tested addition and I hope it can still go > > into the initial upstream process. > > What about the (incremental?) change that Thomas Wagner posted? > > https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de That settles it :) This change needs to got into -next and 5.11.
On 12/5/20 9:33 PM, Jakub Kicinski wrote: >> What about the (incremental?) change that Thomas Wagner posted? >> >> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de > > That settles it :) This change needs to got into -next and 5.11. Ok. Can you take patch 1, which is a real fix: https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ Marc
On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote: > On 12/5/20 9:33 PM, Jakub Kicinski wrote: > >> What about the (incremental?) change that Thomas Wagner posted? > >> > >> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de > > > > That settles it :) This change needs to got into -next and 5.11. > > Ok. Can you take patch 1, which is a real fix: > > https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ Sure! Applied that one from the ML (I assumed that's what you meant). Thanks!
On 12/5/20 10:09 PM, Jakub Kicinski wrote: > On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote: >> On 12/5/20 9:33 PM, Jakub Kicinski wrote: >>>> What about the (incremental?) change that Thomas Wagner posted? >>>> >>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de >>> >>> That settles it :) This change needs to got into -next and 5.11. >> >> Ok. Can you take patch 1, which is a real fix: >> >> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ > > Sure! Applied that one from the ML (I assumed that's what you meant). Thanks, exactly that one. regards, Marc
On 05.12.20 22:09, Jakub Kicinski wrote: > On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote: >> On 12/5/20 9:33 PM, Jakub Kicinski wrote: >>>> What about the (incremental?) change that Thomas Wagner posted? >>>> >>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de >>> >>> That settles it :) This change needs to got into -next and 5.11. >> >> Ok. Can you take patch 1, which is a real fix: >> >> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ > > Sure! Applied that one from the ML (I assumed that's what you meant). > I just double-checked this mail and in fact the second patch from Marc's pull request was a real fix too: https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/ Btw. the missing feature which was added for completeness of the ISOTP implementation has now also integrated the improvement suggested by Thomas Wagner: https://lore.kernel.org/linux-can/20201206144731.4609-1-socketcan@hartkopp.net/T/#u Would be cool if it could go into the initial iso-tp contribution as 5.10 becomes a long-term kernel. But I don't want to be pushy - treat it as your like. Many thanks, Oliver
On Tue, 8 Dec 2020 13:54:28 +0100 Oliver Hartkopp wrote: > On 05.12.20 22:09, Jakub Kicinski wrote: > > On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote: > >> On 12/5/20 9:33 PM, Jakub Kicinski wrote: > >>>> What about the (incremental?) change that Thomas Wagner posted? > >>>> > >>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de > >>> > >>> That settles it :) This change needs to got into -next and 5.11. > >> > >> Ok. Can you take patch 1, which is a real fix: > >> > >> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ > > > > Sure! Applied that one from the ML (I assumed that's what you meant). > > I just double-checked this mail and in fact the second patch from Marc's > pull request was a real fix too: > > https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/ Ack, I thought it was a fix to some existing code but it's a fix to ISO-TP so we should probably get it in before someone start depending on existing behavior - Marc should I apply that one directly, too? > Btw. the missing feature which was added for completeness of the ISOTP > implementation has now also integrated the improvement suggested by > Thomas Wagner: > > https://lore.kernel.org/linux-can/20201206144731.4609-1-socketcan@hartkopp.net/T/#u > > Would be cool if it could go into the initial iso-tp contribution as > 5.10 becomes a long-term kernel. > > But I don't want to be pushy - treat it as your like. I think Linus wants to release 5.10 so that the merge window doesn't overlap with Christmas too much. Let's not push our luck.
On 12/8/20 7:07 PM, Jakub Kicinski wrote: > On Tue, 8 Dec 2020 13:54:28 +0100 Oliver Hartkopp wrote: >> On 05.12.20 22:09, Jakub Kicinski wrote: >>> On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote: >>>> On 12/5/20 9:33 PM, Jakub Kicinski wrote: >>>>>> What about the (incremental?) change that Thomas Wagner posted? >>>>>> >>>>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de >>>>> >>>>> That settles it :) This change needs to got into -next and 5.11. >>>> >>>> Ok. Can you take patch 1, which is a real fix: >>>> >>>> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/ >>> >>> Sure! Applied that one from the ML (I assumed that's what you meant). >> >> I just double-checked this mail and in fact the second patch from Marc's >> pull request was a real fix too: >> >> https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/ > > Ack, I thought it was a fix to some existing code but it's a fix to > ISO-TP so we should probably get it in before someone start depending > on existing behavior - Marc should I apply that one directly, too? Yes, please take that patch directly. Marc
diff --git a/include/uapi/linux/can/isotp.h b/include/uapi/linux/can/isotp.h index 7793b26aa154..c55935b64ccc 100644 --- a/include/uapi/linux/can/isotp.h +++ b/include/uapi/linux/can/isotp.h @@ -135,7 +135,7 @@ struct can_isotp_ll_options { #define CAN_ISOTP_FORCE_RXSTMIN 0x100 /* ignore CFs depending on rx stmin */ #define CAN_ISOTP_RX_EXT_ADDR 0x200 /* different rx extended addressing */ #define CAN_ISOTP_WAIT_TX_DONE 0x400 /* wait for tx completion */ - +#define CAN_ISOTP_SF_BROADCAST 0x800 /* 1-to-N functional addressing */ /* default values */ diff --git a/net/can/isotp.c b/net/can/isotp.c index 26bdc3c20b7e..5ce26e568f16 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -865,6 +865,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) if (!size || size > MAX_MSG_LENGTH) return -EINVAL; + /* take care of a potential SF_DL ESC offset for TX_DL > 8 */ + off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0; + + /* does the given data fit into a single frame for SF_BROADCAST? */ + if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) && + (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) + return -EINVAL; + err = memcpy_from_msg(so->tx.buf, msg, size); if (err < 0) return err; @@ -891,9 +899,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) cf = (struct canfd_frame *)skb->data; skb_put(skb, so->ll.mtu); - /* take care of a potential SF_DL ESC offset for TX_DL > 8 */ - off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0; - /* check for single frame transmission depending on TX_DL */ if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) { /* The message size generally fits into a SingleFrame - good. @@ -1016,7 +1021,7 @@ static int isotp_release(struct socket *sock) hrtimer_cancel(&so->rxtimer); /* remove current filters & unregister */ - if (so->bound) { + if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) { if (so->ifindex) { struct net_device *dev; @@ -1052,6 +1057,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) struct net_device *dev; int err = 0; int notify_enetdown = 0; + int do_rx_reg = 1; if (len < CAN_REQUIRED_SIZE(struct sockaddr_can, can_addr.tp)) return -EINVAL; @@ -1066,6 +1072,10 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (!addr->can_ifindex) return -ENODEV; + /* do not register frame reception for functional addressing */ + if (so->opt.flags & CAN_ISOTP_SF_BROADCAST) + do_rx_reg = 0; + lock_sock(sk); if (so->bound && addr->can_ifindex == so->ifindex && @@ -1093,13 +1103,14 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len) ifindex = dev->ifindex; - can_rx_register(net, dev, addr->can_addr.tp.rx_id, - SINGLE_MASK(addr->can_addr.tp.rx_id), isotp_rcv, sk, - "isotp", sk); + if (do_rx_reg) + can_rx_register(net, dev, addr->can_addr.tp.rx_id, + SINGLE_MASK(addr->can_addr.tp.rx_id), + isotp_rcv, sk, "isotp", sk); dev_put(dev); - if (so->bound) { + if (so->bound && do_rx_reg) { /* unregister old filter */ if (so->ifindex) { dev = dev_get_by_index(net, so->ifindex); @@ -1302,7 +1313,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg, case NETDEV_UNREGISTER: lock_sock(sk); /* remove current filters & unregister */ - if (so->bound) + if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) can_rx_unregister(dev_net(dev), dev, so->rxid, SINGLE_MASK(so->rxid), isotp_rcv, sk);