The original code is okay. Passing zero to ERR_PTR() is intentional. On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > to the skb local variable. This may be an ERR_PTR. > > A little later the following is executed, which seems to be a > possible dereference of an ERR_PTR. > > total_bytes += skb->len; There is an IS_ERR() check in igc_cleanup_headers() which prevents this. Sort of tricky to see. Do you have the cross function database set up? If so then Smatch shouldn't warn about this dereference. > > Avoid this problem by continuing the loop in which all of the > above occurs once the handling of the NULL case completes. > > This proposed fix is speculative - I do not have deep knowledge of this > driver. And I am concerned about the effect of skipping the following > logic: > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > cleaned_count++; > > Flagged by Smatch as: > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' Linus once complained to me that this check is bogus and passing zero to ERR_PTR() is fine and an intended use case. But actually this test does really find a lot of bugs. I think for new warnings it is less than 10% false positives. But we fix the bugs so warnings which are over three month old are probably 97% false positives. regards, dan carpenter
