Re: [PATCH net-next] ipv4/fib: send RTM_DELROUTE notify when flush fib

[Ido Schimmel <idosch@idosch.org>]

On Fri, Aug 04, 2023 at 04:09:08PM +0800, Hangbin Liu wrote:
> I reconsidered this issue this week. As we can't get the device status in
> fib_table_flush(). How about adding another flag to track the deleted src
> entries. e.g. RTNH_F_UNRESOLVED. Which is only used in ipmr currently.
> When the src route address is deleted, the route entry also could be
> considered as unresolved. With this idea, the patch could be like:
> 
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 51c13cf9c5ae..5c41d34ab447 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -420,7 +420,7 @@ struct rtnexthop {
>  #define RTNH_F_ONLINK          4       /* Gateway is forced on link    */
>  #define RTNH_F_OFFLOAD         8       /* Nexthop is offloaded */
>  #define RTNH_F_LINKDOWN                16      /* carrier-down on nexthop */
> -#define RTNH_F_UNRESOLVED      32      /* The entry is unresolved (ipmr) */
> +#define RTNH_F_UNRESOLVED      32      /* The entry is unresolved (ipmr/dead src) */
>  #define RTNH_F_TRAP            64      /* Nexthop is trapping packets */
> 
>  #define RTNH_COMPARE_MASK      (RTNH_F_DEAD | RTNH_F_LINKDOWN | \

I'm not sure we need to reinterpret the meaning of this uAPI flag. The
FIB info structure currently looks like this:

struct fib_info {
        struct hlist_node          fib_hash;             /*     0    16 */
        struct hlist_node          fib_lhash;            /*    16    16 */
        struct list_head           nh_list;              /*    32    16 */
        struct net *               fib_net;              /*    48     8 */
        refcount_t                 fib_treeref;          /*    56     4 */
        refcount_t                 fib_clntref;          /*    60     4 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        unsigned int               fib_flags;            /*    64     4 */
        unsigned char              fib_dead;             /*    68     1 */
        unsigned char              fib_protocol;         /*    69     1 */
        unsigned char              fib_scope;            /*    70     1 */
        unsigned char              fib_type;             /*    71     1 */
        __be32                     fib_prefsrc;          /*    72     4 */
        u32                        fib_tb_id;            /*    76     4 */
        u32                        fib_priority;         /*    80     4 */

        /* XXX 4 bytes hole, try to pack */

        struct dst_metrics *       fib_metrics;          /*    88     8 */
        int                        fib_nhs;              /*    96     4 */
        bool                       fib_nh_is_v6;         /*   100     1 */
        bool                       nh_updated;           /*   101     1 */

        /* XXX 2 bytes hole, try to pack */

        struct nexthop *           nh;                   /*   104     8 */
        struct callback_head       rcu __attribute__((__aligned__(8))); /*   112    16 */
        /* --- cacheline 2 boundary (128 bytes) --- */
        struct fib_nh              fib_nh[];             /*   128     0 */

        /* size: 128, cachelines: 2, members: 21 */
        /* sum members: 122, holes: 2, sum holes: 6 */
        /* forced alignments: 1 */
} __attribute__((__aligned__(8)));

We can instead represent fib_nh_is_v6 and nh_updated using a single bit:

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index a378eff827c7..a91f8a28689a 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -152,8 +152,8 @@ struct fib_info {
 #define fib_rtt fib_metrics->metrics[RTAX_RTT-1]
 #define fib_advmss fib_metrics->metrics[RTAX_ADVMSS-1]
        int                     fib_nhs;
-       bool                    fib_nh_is_v6;
-       bool                    nh_updated;
+       u8                      fib_nh_is_v6:1,
+                               nh_updated:1;
        struct nexthop          *nh;
        struct rcu_head         rcu;
        struct fib_nh           fib_nh[];

And then add another bit there to mark a FIB info that is deleted
because of preferred source address deletion.

I suggest testing with the FIB tests in tools/testing/selftests/net/.

> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 65ba18a91865..a7ef21a6d271 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1883,7 +1883,7 @@ int fib_sync_down_addr(struct net_device *dev, __be32 local)
>                     fi->fib_tb_id != tb_id)
>                         continue;
>                 if (fi->fib_prefsrc == local) {
> -                       fi->fib_flags |= RTNH_F_DEAD;
> +                       fi->fib_flags |= (RTNH_F_DEAD | RTNH_F_UNRESOLVED);
>                         ret++;
>                 }
>         }
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 74d403dbd2b4..88c593967063 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2026,6 +2026,7 @@ void fib_table_flush_external(struct fib_table *tb)
>  int fib_table_flush(struct net *net, struct fib_table *tb, bool flush_all)
>  {
>         struct trie *t = (struct trie *)tb->tb_data;
> +       struct nl_info info = { .nl_net = net };
>         struct key_vector *pn = t->kv;
>         unsigned long cindex = 1;
>         struct hlist_node *tmp;
> @@ -2088,6 +2089,11 @@ int fib_table_flush(struct net *net, struct fib_table *tb, bool flush_all)
> 
>                         fib_notify_alias_delete(net, n->key, &n->leaf, fa,
>                                                 NULL);
> +                       if (fi->fib_flags & RTNH_F_UNRESOLVED) {
> +                               fi->fib_flags &= ~RTNH_F_UNRESOLVED;
> +                               rtmsg_fib(RTM_DELROUTE, htonl(n->key), fa,
> +                                         KEYLENGTH - fa->fa_slen, tb->tb_id, &info, 0);
> +                       }
>                         hlist_del_rcu(&fa->fa_list);
>                         fib_release_info(fa->fa_info);
>                         alias_free_mem_rcu(fa);
> 
> Thanks
> Hangbin