netdev - Re: [PATCH net-next 2/6] igb: Add double-check MTA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 May 2021 14:43:55 +0100
From:   Nick Lowe <nick.lowe@...il.com>
To:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Cc:     "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Siwik, Grzegorz" <grzegorz.siwik@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Switzer, David" <david.switzer@...el.com>
Subject: Re: [PATCH net-next 2/6] igb: Add double-check MTA_REGISTER for i210
 and i211

Hi all,

> > Looks like a potential infinite loop on persistent failure.
> > Also you don't need "is_failed", you can use while (i >= 0), or
> > assign i = hw->mac.mta_reg_count, or consider using a goto.
>
> We will make a follow on patch to address these issues.
>
> Thanks,
> Tony

The patch for this that has been queued is as follows:

+     int failed_cnt = 3;
+     bool is_failed;
+     int i;
+
+     do {
+          is_failed = false;
+          for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
+               if (array_rd32(E1000_MTA, i) != hw->mac.mta_shadow[i]) {
+                    is_failed = true;
+                    array_wr32(E1000_MTA, i, hw->mac.mta_shadow[i]);
+                    wrfl();
+               }
+          }
+          if (is_failed && --failed_cnt <= 0) {
+               hw_dbg("Failed to update MTA_REGISTER, too many retries");
+               break;
+          }
+     } while (is_failed);

https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git/commit/?h=dev-queue&id=9db33b54fb98525e323d0d3f16b01778f17b9493

This will not reset the counter when checking each register and it
will not debug output which register failed, this does not seem
optimal.

Could it make more sense to instead do something like this? (Untested)

+     int i;
+     int attempt;
+     for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
+          for (attempt = 3; attempt >= 1; attempt--) {
+               if (array_rd32(E1000_MTA, i) != hw->mac.mta_shadow[i]) {
+                    array_wr32(E1000_MTA, i, hw->mac.mta_shadow[i]);
+                    wrfl();
+
+                    if (attempt == 1 && array_rd32(E1000_MTA, i) !=
hw->mac.mta_shadow[i]) {
+                         hw_dbg("Failed to update MTA_REGISTER %d,
too many retries\n", i);
+                    }
+               }
+          }
+     }

Best,

Nick