| Message ID | 20201209081604.464084-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: x25: Fix handling of Restart Request and Restart Confirmation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 1 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, 50 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 2020-12-09 09:16, Xie He wrote: > 1. When the x25 module gets loaded, layer 2 may already be running and > connected. In this case, although we are in X25_LINK_STATE_0, we still > need to handle the Restart Request received, rather than ignore it. Hmm... I've never loaded the X.25 module after the interface is UP, but in this case we really have to fix it. > > 2. When we are in X25_LINK_STATE_2, we have already sent a Restart > Request > and is waiting for the Restart Confirmation with t20timer. t20timer > will > restart itself repeatedly forever so it will always be there, as long > as we > are in State 2. So we don't need to check x25_t20timer_pending again. Yeah, you're right, we can actually leave that out. Acked-by: Martin Schiller <ms@dev.tdt.de>
On 2020-12-09 10:52, Martin Schiller wrote: > On 2020-12-09 09:16, Xie He wrote: >> 1. When the x25 module gets loaded, layer 2 may already be running and >> connected. In this case, although we are in X25_LINK_STATE_0, we still >> need to handle the Restart Request received, rather than ignore it. > > Hmm... I've never loaded the X.25 module after the interface is UP, but > in this case we really have to fix it. > This seems to be a regression caused by moving the Layer2 link handling into the lapb driver, which wasn't intended in my original patchset. I also have another patch on my todo list which aims orphan packet handling in the x25_receive_data() function. Maybe it is better to catch the whole thing there. >> >> 2. When we are in X25_LINK_STATE_2, we have already sent a Restart >> Request >> and is waiting for the Restart Confirmation with t20timer. t20timer >> will >> restart itself repeatedly forever so it will always be there, as long >> as we >> are in State 2. So we don't need to check x25_t20timer_pending again. > > Yeah, you're right, we can actually leave that out. > > Acked-by: Martin Schiller <ms@dev.tdt.de>
On Wed, Dec 9, 2020 at 2:31 AM Martin Schiller <ms@dev.tdt.de> wrote: > > >> 1. When the x25 module gets loaded, layer 2 may already be running and > >> connected. In this case, although we are in X25_LINK_STATE_0, we still > >> need to handle the Restart Request received, rather than ignore it. > > > > Hmm... I've never loaded the X.25 module after the interface is UP, but > > in this case we really have to fix it. > > > > This seems to be a regression caused by moving the Layer2 link handling > into the lapb driver, which wasn't intended in my original patchset. > > I also have another patch on my todo list which aims orphan packet > handling in the x25_receive_data() function. Maybe it is better to catch > the whole thing there. OK.. Currently it's not clear to me what your future patches would be. Maybe we can first have this patch applied? Because based on the current code I think this patch is necessary. When you are ready to submit your patches, you can replace my code and we can discuss further.
From: Xie He <xie.he.0141@gmail.com> Date: Wed, 9 Dec 2020 00:16:04 -0800 > 1. When the x25 module gets loaded, layer 2 may already be running and > connected. In this case, although we are in X25_LINK_STATE_0, we still > need to handle the Restart Request received, rather than ignore it. > > 2. When we are in X25_LINK_STATE_2, we have already sent a Restart Request > and is waiting for the Restart Confirmation with t20timer. t20timer will > restart itself repeatedly forever so it will always be there, as long as we > are in State 2. So we don't need to check x25_t20timer_pending again. > > Fixes: d023b2b9ccc2 ("net/x25: fix restart request/confirm handling") > Cc: Martin Schiller <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> Applied, thanks.
On 2020-12-09 21:16, Xie He wrote: > On Wed, Dec 9, 2020 at 2:31 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> >> 1. When the x25 module gets loaded, layer 2 may already be running and >> >> connected. In this case, although we are in X25_LINK_STATE_0, we still >> >> need to handle the Restart Request received, rather than ignore it. >> > >> > Hmm... I've never loaded the X.25 module after the interface is UP, but >> > in this case we really have to fix it. >> > >> >> This seems to be a regression caused by moving the Layer2 link >> handling >> into the lapb driver, which wasn't intended in my original patchset. >> >> I also have another patch on my todo list which aims orphan packet >> handling in the x25_receive_data() function. Maybe it is better to >> catch >> the whole thing there. > > OK.. > > Currently it's not clear to me what your future patches would be. > Maybe we can first have this patch applied? Because based on the > current code I think this patch is necessary. When you are ready to > submit your patches, you can replace my code and we can discuss > further. Yes, that's also the reason why I already acked this patch. We can solve this later a little bit cleaner if necessary. My patch that takes care of the orphaned packets in x25_receive_data() has again a dependency on other patches, especially the patch to configure the neighbor parameters (DCE/DTE, number of channels etc.), which I already sent before but still have to revise. Unfortunately I have only limited time for this topic, so I am not as fast as some people would wish. Sorry for that. Martin
On Wed, Dec 9, 2020 at 10:35 PM Martin Schiller <ms@dev.tdt.de> wrote: > > Yes, that's also the reason why I already acked this patch. We can > solve this later a little bit cleaner if necessary. > > My patch that takes care of the orphaned packets in x25_receive_data() > has again a dependency on other patches, especially the patch to > configure the neighbor parameters (DCE/DTE, number of channels etc.), > which I already sent before but still have to revise. > > Unfortunately I have only limited time for this topic, so I am not as > fast as some people would wish. Sorry for that. OK. Thanks! I appreciate your work! Code needs to have specialist developers like you to keep it alive and evolving. I understand you have limited time. Please take your time. Thanks!
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c index f92073f3cb11..57a81100c5da 100644 --- a/net/x25/x25_link.c +++ b/net/x25/x25_link.c @@ -58,11 +58,6 @@ static inline void x25_stop_t20timer(struct x25_neigh *nb) del_timer(&nb->t20timer); } -static inline int x25_t20timer_pending(struct x25_neigh *nb) -{ - return timer_pending(&nb->t20timer); -} - /* * This handles all restart and diagnostic frames. */ @@ -70,17 +65,20 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh *nb, unsigned short frametype) { struct sk_buff *skbn; - int confirm; switch (frametype) { case X25_RESTART_REQUEST: switch (nb->state) { + case X25_LINK_STATE_0: + /* This can happen when the x25 module just gets loaded + * and doesn't know layer 2 has already connected + */ + nb->state = X25_LINK_STATE_3; + x25_transmit_restart_confirmation(nb); + break; case X25_LINK_STATE_2: - confirm = !x25_t20timer_pending(nb); x25_stop_t20timer(nb); nb->state = X25_LINK_STATE_3; - if (confirm) - x25_transmit_restart_confirmation(nb); break; case X25_LINK_STATE_3: /* clear existing virtual calls */ @@ -94,13 +92,8 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh *nb, case X25_RESTART_CONFIRMATION: switch (nb->state) { case X25_LINK_STATE_2: - if (x25_t20timer_pending(nb)) { - x25_stop_t20timer(nb); - nb->state = X25_LINK_STATE_3; - } else { - x25_transmit_restart_request(nb); - x25_start_t20timer(nb); - } + x25_stop_t20timer(nb); + nb->state = X25_LINK_STATE_3; break; case X25_LINK_STATE_3: /* clear existing virtual calls */
1. When the x25 module gets loaded, layer 2 may already be running and connected. In this case, although we are in X25_LINK_STATE_0, we still need to handle the Restart Request received, rather than ignore it. 2. When we are in X25_LINK_STATE_2, we have already sent a Restart Request and is waiting for the Restart Confirmation with t20timer. t20timer will restart itself repeatedly forever so it will always be there, as long as we are in State 2. So we don't need to check x25_t20timer_pending again. Fixes: d023b2b9ccc2 ("net/x25: fix restart request/confirm handling") Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- net/x25/x25_link.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-)