Re: [PATCH net 00/10] net: rmnet: fix several bugs

[subashab@codeaurora.org]

On 2020-02-26 10:41, Taehee Yoo wrote:
> This patchset is to fix several bugs in RMNET module.
> 
> 1. The first patch fixes NULL-ptr-deref in rmnet_newlink().
> When rmnet interface is being created, it uses IFLA_LINK
> without checking NULL.
> So, if userspace doesn't set IFLA_LINK, panic will occur.
> In this patch, checking NULL pointer code is added.
> 
> 2. The second patch adds module alias.
> In the current rmnet code, there is no module alias.
> So, RTNL couldn't load rmnet module automatically.
> 
> 3. The third patch fixes NULL-ptr-deref in rmnet_changelink().
> To get real device in rmnet_changelink(), it uses IFLA_LINK.
> But, IFLA_LINK should not be used in rmnet_changelink().
> 
> 4. The fourth patch fixes suspicious RCU usage in rmnet_get_port().
> rmnet_get_port() uses rcu_dereference_rtnl().
> But, rmnet_get_port() is used by datapath.
> So, rcu_dereference() should be used instead of rcu_dereference_rtnl().
> 
> 5. The fifth patch fixes suspicious RCU usage in
> rmnet_force_unassociate_device().
> RCU critical section should not be scheduled.
> But, unregister_netdevice_queue() in the 
> rmnet_force_unassociate_device()
> would be scheduled.
> So, the RCU warning occurs.
> In this patch, the rcu_read_lock() in the 
> rmnet_force_unassociate_device()
> is removed because it's unnecessary.
> 
> 6. The sixth patch adds extack error messages when command fails
> When rmnet netlink command fails, it doesn't print any error message.
> So, users couldn't know the exact reason.
> 
> 7. The seventh patch fixes duplicate MUX ID case.
> RMNET MUX ID is unique.
> So, rmnet interface isn't allowed to be created, which have
> a duplicate MUX ID.
> But, only rmnet_newlink() checks this condition, rmnet_changelink()
> doesn't check this.
> So, duplicate MUX ID case would happen.
> 
> 8. The eighth patch fixes upper/lower interface relationship problems.
> When IFLA_LINK is used, the upper/lower infrastructure should be used.
> Because it checks the maximum depth of upper/lower interfaces and it 
> also
> checks circular interface relationship, etc.
> In this patch, netdev_upper_dev_link() is used.
> 
> 9. The ninth patch fixes bridge related problems.
> a) ->ndo_del_slave() doesn't work.
> b) It couldn't detect circular upper/lower interface relationship.
> c) It couldn't prevent stack overflow because of too deep depth
> of upper/lower interface
> d) It doesn't check the number of lower interfaces.
> e) Panics because of several reasons.
> These problems are actually the same problem.
> So, this patch fixes these problems.
> 
> 10. The tenth patch fixes packet forwarding issue in bridge mode
> Packet forwarding is not working in rmnet bridge mode.
> Because when a packet is forwarded, skb_push() for an ethernet header
> is needed. But it doesn't call skb_push().
> So, the ethernet header will be lost.
> 
> Taehee Yoo (10):
>   net: rmnet: fix NULL pointer dereference in rmnet_newlink()
>   net: rmnet: add missing module alias
>   net: rmnet: fix NULL pointer dereference in rmnet_changelink()
>   net: rmnet: fix suspicious RCU usage
>   net: rmnet: remove rcu_read_lock in rmnet_force_unassociate_device()
>   net: rmnet: print error message when command fails
>   net: rmnet: do not allow to change mux id if mux id is duplicated
>   net: rmnet: use upper/lower device infrastructure
>   net: rmnet: fix bridge mode bugs
>   net: rmnet: fix packet forwarding in rmnet bridge mode
> 
>  .../ethernet/qualcomm/rmnet/rmnet_config.c    | 210 +++++++++---------
>  .../ethernet/qualcomm/rmnet/rmnet_config.h    |   3 +-
>  .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |   7 +-
>  .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  20 +-
>  .../net/ethernet/qualcomm/rmnet/rmnet_vnd.h   |   4 +-
>  5 files changed, 122 insertions(+), 122 deletions(-)

Hi Taehee

Thanks for sending the fixes.
I have some minor comments on few of the patches.