* [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt
@ 2019-06-30 20:46 Andreas Steinmetz
2019-07-01 2:05 ` Willem de Bruijn
0 siblings, 1 reply; 5+ messages in thread
From: Andreas Steinmetz @ 2019-06-30 20:46 UTC (permalink / raw)
To: netdev; +Cc: Sabrina Dubroca
At this point, skb could only be a valid pointer, so this patch does
not introduce any functional change.
Signed-off-by: Andreas Steinmetz <ast@domdv.de>
--- a/drivers/net/macsec.c 2019-06-30 22:05:17.785683634 +0200
+++ b/drivers/net/macsec.c 2019-06-30 22:05:20.526171178 +0200
@@ -1205,21 +1205,22 @@
/* Disabled && !changed text => skip validation */
if (hdr->tci_an & MACSEC_TCI_C ||
- secy->validate_frames != MACSEC_VALIDATE_DISABLED)
+ secy->validate_frames != MACSEC_VALIDATE_DISABLED) {
skb = macsec_decrypt(skb, dev, rx_sa, sci, secy);
- if (IS_ERR(skb)) {
- /* the decrypt callback needs the reference */
- if (PTR_ERR(skb) != -EINPROGRESS) {
- macsec_rxsa_put(rx_sa);
- macsec_rxsc_put(rx_sc);
+ if (IS_ERR(skb)) {
+ /* the decrypt callback needs the reference */
+ if (PTR_ERR(skb) != -EINPROGRESS) {
+ macsec_rxsa_put(rx_sa);
+ macsec_rxsc_put(rx_sc);
+ }
+ rcu_read_unlock();
+ return RX_HANDLER_CONSUMED;
}
- rcu_read_unlock();
- return RX_HANDLER_CONSUMED;
- }
- if (!macsec_post_decrypt(skb, secy, pn))
- goto drop;
+ if (!macsec_post_decrypt(skb, secy, pn))
+ goto drop;
+ }
deliver:
macsec_finalize_skb(skb, secy->icv_len,
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt 2019-06-30 20:46 [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt Andreas Steinmetz @ 2019-07-01 2:05 ` Willem de Bruijn 2019-07-01 13:21 ` Sabrina Dubroca 0 siblings, 1 reply; 5+ messages in thread From: Willem de Bruijn @ 2019-07-01 2:05 UTC (permalink / raw) To: Andreas Steinmetz; +Cc: Network Development, Sabrina Dubroca On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de> wrote: > > At this point, skb could only be a valid pointer, so this patch does > not introduce any functional change. Previously, macsec_post_decrypt could be called on the original skb if the initial condition was false and macsec_decrypt is skipped. That was probably unintended. Either way, then this is a functional change, and perhaps a bugfix? > Signed-off-by: Andreas Steinmetz <ast@domdv.de> > > --- a/drivers/net/macsec.c 2019-06-30 22:05:17.785683634 +0200 > +++ b/drivers/net/macsec.c 2019-06-30 22:05:20.526171178 +0200 > @@ -1205,21 +1205,22 @@ > > /* Disabled && !changed text => skip validation */ > if (hdr->tci_an & MACSEC_TCI_C || > - secy->validate_frames != MACSEC_VALIDATE_DISABLED) > + secy->validate_frames != MACSEC_VALIDATE_DISABLED) { > skb = macsec_decrypt(skb, dev, rx_sa, sci, secy); > > - if (IS_ERR(skb)) { > - /* the decrypt callback needs the reference */ > - if (PTR_ERR(skb) != -EINPROGRESS) { > - macsec_rxsa_put(rx_sa); > - macsec_rxsc_put(rx_sc); > + if (IS_ERR(skb)) { > + /* the decrypt callback needs the reference */ > + if (PTR_ERR(skb) != -EINPROGRESS) { > + macsec_rxsa_put(rx_sa); > + macsec_rxsc_put(rx_sc); > + } > + rcu_read_unlock(); > + return RX_HANDLER_CONSUMED; > } > - rcu_read_unlock(); > - return RX_HANDLER_CONSUMED; > - } > > - if (!macsec_post_decrypt(skb, secy, pn)) > - goto drop; > + if (!macsec_post_decrypt(skb, secy, pn)) > + goto drop; > + } > > deliver: > macsec_finalize_skb(skb, secy->icv_len, > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt 2019-07-01 2:05 ` Willem de Bruijn @ 2019-07-01 13:21 ` Sabrina Dubroca 2019-07-02 4:37 ` Andreas Steinmetz 0 siblings, 1 reply; 5+ messages in thread From: Sabrina Dubroca @ 2019-07-01 13:21 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Andreas Steinmetz, Network Development 2019-06-30, 22:05:41 -0400, Willem de Bruijn wrote: > On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de> wrote: > > > > At this point, skb could only be a valid pointer, so this patch does > > not introduce any functional change. > > Previously, macsec_post_decrypt could be called on the original skb if > the initial condition was false and macsec_decrypt is skipped. That > was probably unintended. Either way, then this is a functional change, > and perhaps a bugfix? Ouch, I missed that when Andreas sent me that patch before. No, it is actually intended. If we skip macsec_decrypt(), we should still account for that packet in the InPktsUnchecked/InPktsDelayed counters. That's in Figure 10-5 in the standard. Thanks for catching this, Willem. That patch should only move the IS_ERR(skb) case under the block where macsec_decrypt() is called, but not move the call to macsec_post_decrypt(). > > Signed-off-by: Andreas Steinmetz <ast@domdv.de> > > > > --- a/drivers/net/macsec.c 2019-06-30 22:05:17.785683634 +0200 > > +++ b/drivers/net/macsec.c 2019-06-30 22:05:20.526171178 +0200 > > @@ -1205,21 +1205,22 @@ > > > > /* Disabled && !changed text => skip validation */ > > if (hdr->tci_an & MACSEC_TCI_C || > > - secy->validate_frames != MACSEC_VALIDATE_DISABLED) > > + secy->validate_frames != MACSEC_VALIDATE_DISABLED) { > > skb = macsec_decrypt(skb, dev, rx_sa, sci, secy); > > > > - if (IS_ERR(skb)) { > > - /* the decrypt callback needs the reference */ > > - if (PTR_ERR(skb) != -EINPROGRESS) { > > - macsec_rxsa_put(rx_sa); > > - macsec_rxsc_put(rx_sc); > > + if (IS_ERR(skb)) { > > + /* the decrypt callback needs the reference */ > > + if (PTR_ERR(skb) != -EINPROGRESS) { > > + macsec_rxsa_put(rx_sa); > > + macsec_rxsc_put(rx_sc); > > + } > > + rcu_read_unlock(); > > + return RX_HANDLER_CONSUMED; > > } > > - rcu_read_unlock(); > > - return RX_HANDLER_CONSUMED; > > - } > > > > - if (!macsec_post_decrypt(skb, secy, pn)) > > - goto drop; > > + if (!macsec_post_decrypt(skb, secy, pn)) > > + goto drop; > > + } > > > > deliver: > > macsec_finalize_skb(skb, secy->icv_len, > > -- Sabrina ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt 2019-07-01 13:21 ` Sabrina Dubroca @ 2019-07-02 4:37 ` Andreas Steinmetz 2019-07-02 14:35 ` Willem de Bruijn 0 siblings, 1 reply; 5+ messages in thread From: Andreas Steinmetz @ 2019-07-02 4:37 UTC (permalink / raw) To: Sabrina Dubroca, Willem de Bruijn; +Cc: Network Development Ouch, I missed that when Andreas sent me that patch before. No, it is > actually intended. If we skip macsec_decrypt(), we should still > account for that packet in the InPktsUnchecked/InPktsDelayed > counters. That's in Figure 10-5 in the standard. > > Thanks for catching this, Willem. That patch should only move the > IS_ERR(skb) case under the block where macsec_decrypt() is called, but > not move the call to macsec_post_decrypt(). Updated patch below. Signed-off-by: Andreas Steinmetz <ast@domdv.de> --- a/drivers/net/macsec.c 2019-07-02 06:31:27.550120145 +0200 +++ b/drivers/net/macsec.c 2019-07-02 06:33:38.637599529 +0200 @@ -1205,17 +1205,18 @@ /* Disabled && !changed text => skip validation */ if (hdr->tci_an & MACSEC_TCI_C || - secy->validate_frames != MACSEC_VALIDATE_DISABLED) + secy->validate_frames != MACSEC_VALIDATE_DISABLED) { skb = macsec_decrypt(skb, dev, rx_sa, sci, secy); - if (IS_ERR(skb)) { - /* the decrypt callback needs the reference */ - if (PTR_ERR(skb) != -EINPROGRESS) { - macsec_rxsa_put(rx_sa); - macsec_rxsc_put(rx_sc); + if (IS_ERR(skb)) { + /* the decrypt callback needs the reference */ + if (PTR_ERR(skb) != -EINPROGRESS) { + macsec_rxsa_put(rx_sa); + macsec_rxsc_put(rx_sc); + } + rcu_read_unlock(); + return RX_HANDLER_CONSUMED; } - rcu_read_unlock(); - return RX_HANDLER_CONSUMED; } if (!macsec_post_decrypt(skb, secy, pn)) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt 2019-07-02 4:37 ` Andreas Steinmetz @ 2019-07-02 14:35 ` Willem de Bruijn 0 siblings, 0 replies; 5+ messages in thread From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw) To: Andreas Steinmetz; +Cc: Sabrina Dubroca, Network Development On Tue, Jul 2, 2019 at 12:38 AM Andreas Steinmetz <ast@domdv.de> wrote: > > Ouch, I missed that when Andreas sent me that patch before. No, it is > > actually intended. If we skip macsec_decrypt(), we should still > > account for that packet in the InPktsUnchecked/InPktsDelayed > > counters. That's in Figure 10-5 in the standard. > > > > Thanks for catching this, Willem. That patch should only move the > > IS_ERR(skb) case under the block where macsec_decrypt() is called, but > > not move the call to macsec_post_decrypt(). > > Updated patch below. > > Signed-off-by: Andreas Steinmetz <ast@domdv.de> When making a change in a patch set, please resubmit the entire patchset (with a v2, and record the changelog). ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-02 14:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-30 20:46 [PATCH net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt Andreas Steinmetz 2019-07-01 2:05 ` Willem de Bruijn 2019-07-01 13:21 ` Sabrina Dubroca 2019-07-02 4:37 ` Andreas Steinmetz 2019-07-02 14:35 ` Willem de Bruijn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox